Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

2016-09-29 Thread Radim Krčmář 
2016-09-29 18:06+0200, Igor Mammedov:
> On Thu, 29 Sep 2016 15:18:36 +0200
> Paolo Bonzini  wrote:
>> On 29/09/2016 13:23, Radim Krčmář wrote:
>> > Cluster x2APIC cannot work without KVM's x2apic API when the maximal
>> > APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
>> > forbid other APICs and also the old KVM case with less than 9, to
>> > simplify the code.
>> > 
>> > There is no point in enabling EIM in forbidden APICs, so we keep it
>> > enabled only for the KVM APIC;  unconditionally, because making the
>> > option depend on KVM version would be a maintanance burden.
>> > 
>> > Signed-off-by: Radim Krčmář 
>> > ---
>> > v2:
>> >  * adapt to new intr_eim parameter
>> >  * provide first linux version that has x2apic api
>> >  * disable QEMU's LAPIC
>> > ---
>> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> > @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error 
>> > **errp)
>> >  s->intr_eim = ON_OFF_AUTO_OFF;
>> >  }
>> >  if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>> > -s->intr_eim = ON_OFF_AUTO_ON;
>> > +s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
>> > +  : ON_OFF_AUTO_OFF;
>> > +}
>> > +if (s->intr_eim == ON_OFF_AUTO_ON) {
>> > +if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
>> > +error_report("intel-iommu,eim=on requires support on the KVM 
>> > side "
>> > + "(X2APIC_API, first shipped in v4.7).");
>> > +exit(1);  
>> 
>> Please use error_setg and return instead (same in patches 4 and 5).
> Radim's version is consistent with error handling used throughout the file.
> If we are to use preferred error_setg() here that preceding cleanup
> patch is in order to convert error_reports to error_setg.

There is one more error_report() in the file and it doesn't have
"Error **" -- I'll leave it be and change the rest.
It amounts to one extra patch that before [4/7] (could be squashed too).

>> > +}
>> > +if (!kvm_irqchip_in_kernel()) {
>> > +error_report("intel-iommu,eim=on requires "
>> > + "accel=kvm,kernel-irqchip=split.");
> this prints:
>  -device intel-iommu,intremap=on,eim=on: intel-iommu,eim=on requires 
> accel=kvm,kernel-irqchip=split
> so 'intel-iommu,' not really needed, the same would happen with error_setg()

Yeah, really unreadable, thanks.

Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

2016-09-29 Thread Paolo Bonzini 


On 29/09/2016 18:56, Radim Krčmář wrote:
> 2016-09-29 18:06+0200, Igor Mammedov:
>> On Thu, 29 Sep 2016 15:18:36 +0200
>> Paolo Bonzini  wrote:
>>> On 29/09/2016 13:23, Radim Krčmář wrote:
 Cluster x2APIC cannot work without KVM's x2apic API when the maximal
 APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
 forbid other APICs and also the old KVM case with less than 9, to
 simplify the code.

 There is no point in enabling EIM in forbidden APICs, so we keep it
 enabled only for the KVM APIC;  unconditionally, because making the
 option depend on KVM version would be a maintanance burden.

 Signed-off-by: Radim Krčmář 
 ---
 v2:
  * adapt to new intr_eim parameter
  * provide first linux version that has x2apic api
  * disable QEMU's LAPIC
 ---
 diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
 @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error 
 **errp)
  s->intr_eim = ON_OFF_AUTO_OFF;
  }
  if (s->intr_eim == ON_OFF_AUTO_AUTO) {
 -s->intr_eim = ON_OFF_AUTO_ON;
 +s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
 +  : ON_OFF_AUTO_OFF;
 +}
 +if (s->intr_eim == ON_OFF_AUTO_ON) {
 +if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
 +error_report("intel-iommu,eim=on requires support on the KVM 
 side "
 + "(X2APIC_API, first shipped in v4.7).");
 +exit(1);  
>>>
>>> Please use error_setg and return instead (same in patches 4 and 5).
>> Radim's version is consistent with error handling used throughout the file.
>> If we are to use preferred error_setg() here that preceding cleanup
>> patch is in order to convert error_reports to error_setg.
> 
> There is one more error_report() in the file and it doesn't have
> "Error **" -- I'll leave it be and change the rest.
> It amounts to one extra patch that before [4/7] (could be squashed too).

No problem then, keep using error_report I guess.

Paolo


 +}
 +if (!kvm_irqchip_in_kernel()) {
 +error_report("intel-iommu,eim=on requires "
 + "accel=kvm,kernel-irqchip=split.");
>> this prints:
>>  -device intel-iommu,intremap=on,eim=on: intel-iommu,eim=on requires 
>> accel=kvm,kernel-irqchip=split
>> so 'intel-iommu,' not really needed, the same would happen with error_setg()
> 
> Yeah, really unreadable, thanks.
>

Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

2016-09-29 Thread Igor Mammedov 
On Thu, 29 Sep 2016 15:18:36 +0200
Paolo Bonzini  wrote:

> On 29/09/2016 13:23, Radim Krčmář wrote:
> > Cluster x2APIC cannot work without KVM's x2apic API when the maximal
> > APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
> > forbid other APICs and also the old KVM case with less than 9, to
> > simplify the code.
> > 
> > There is no point in enabling EIM in forbidden APICs, so we keep it
> > enabled only for the KVM APIC;  unconditionally, because making the
> > option depend on KVM version would be a maintanance burden.
> > 
> > Signed-off-by: Radim Krčmář 
> > ---
> > v2:
> >  * adapt to new intr_eim parameter
> >  * provide first linux version that has x2apic api
> >  * disable QEMU's LAPIC
> > ---
> >  hw/i386/intel_iommu.c  | 16 +++-
> >  target-i386/kvm-stub.c |  5 +
> >  target-i386/kvm.c  | 13 +
> >  target-i386/kvm_i386.h |  1 +
> >  4 files changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 47141cea64f4..b1afe5b133e0 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -32,6 +32,7 @@
> >  #include "hw/pci-host/q35.h"
> >  #include "sysemu/kvm.h"
> >  #include "hw/i386/apic_internal.h"
> > +#include "kvm_i386.h"
> >  
> >  /*#define DEBUG_INTEL_IOMMU*/
> >  #ifdef DEBUG_INTEL_IOMMU
> > @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error 
> > **errp)
> >  s->intr_eim = ON_OFF_AUTO_OFF;
> >  }
> >  if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> > -s->intr_eim = ON_OFF_AUTO_ON;
> > +s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
> > +  : ON_OFF_AUTO_OFF;
> > +}
> > +if (s->intr_eim == ON_OFF_AUTO_ON) {
> > +if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
> > +error_report("intel-iommu,eim=on requires support on the KVM 
> > side "
> > + "(X2APIC_API, first shipped in v4.7).");
> > +exit(1);  
> 
> Please use error_setg and return instead (same in patches 4 and 5).
Radim's version is consistent with error handling used throughout the file.
If we are to use preferred error_setg() here that preceding cleanup
patch is in order to convert error_reports to error_setg.

> 
> Paolo
> 
> > +}
> > +if (!kvm_irqchip_in_kernel()) {
> > +error_report("intel-iommu,eim=on requires "
> > + "accel=kvm,kernel-irqchip=split.");
this prints:
 -device intel-iommu,intremap=on,eim=on: intel-iommu,eim=on requires 
accel=kvm,kernel-irqchip=split
so 'intel-iommu,' not really needed, the same would happen with error_setg()

> > +exit(1);
> > +}
> >  }
[...]

Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

2016-09-29 Thread Igor Mammedov 
On Thu, 29 Sep 2016 13:23:28 +0200
Radim Krčmář  wrote:

> Cluster x2APIC cannot work without KVM's x2apic API when the maximal
> APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
> forbid other APICs and also the old KVM case with less than 9, to
> simplify the code.
> 
> There is no point in enabling EIM in forbidden APICs, so we keep it
> enabled only for the KVM APIC;  unconditionally, because making the
> option depend on KVM version would be a maintanance burden.
> 
> Signed-off-by: Radim Krčmář 
> ---
> v2:
>  * adapt to new intr_eim parameter
>  * provide first linux version that has x2apic api
>  * disable QEMU's LAPIC
> ---
>  hw/i386/intel_iommu.c  | 16 +++-
>  target-i386/kvm-stub.c |  5 +
>  target-i386/kvm.c  | 13 +
>  target-i386/kvm_i386.h |  1 +
>  4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 47141cea64f4..b1afe5b133e0 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -32,6 +32,7 @@
>  #include "hw/pci-host/q35.h"
>  #include "sysemu/kvm.h"
>  #include "hw/i386/apic_internal.h"
> +#include "kvm_i386.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>  s->intr_eim = ON_OFF_AUTO_OFF;
>  }
>  if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> -s->intr_eim = ON_OFF_AUTO_ON;
> +s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
> +  : ON_OFF_AUTO_OFF;
> +}
> +if (s->intr_eim == ON_OFF_AUTO_ON) {
> +if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
> +error_report("intel-iommu,eim=on requires support on the KVM 
> side "
> + "(X2APIC_API, first shipped in v4.7).");
> +exit(1);
> +}
> +if (!kvm_irqchip_in_kernel()) {
> +error_report("intel-iommu,eim=on requires "
> + "accel=kvm,kernel-irqchip=split.");
> +exit(1);
> +}
>  }
>  
>  memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
> diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
> index cdf15061091d..bda4dc2f0c57 100644
> --- a/target-i386/kvm-stub.c
> +++ b/target-i386/kvm-stub.c
> @@ -25,6 +25,11 @@ bool kvm_has_smm(void)
>  return 1;
>  }
>  
> +bool kvm_enable_x2apic(void)
> +{
> +return false;
> +}
> +
>  /* This function is only called inside conditionals which we
>   * rely on the compiler to optimize out when CONFIG_KVM is not
>   * defined.
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ee1f53e56953..0fd664648665 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -122,6 +122,19 @@ bool kvm_allows_irq0_override(void)
>  return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
>  }
>  
> +static bool kvm_x2apic_api_set_flags(uint64_t flags)
> +{
> +KVMState *s = KVM_STATE(current_machine->accelerator);
> +
> +return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
> +}
it's called only from one place so why not to inline it in the caller?

> +
> +bool kvm_enable_x2apic(void)
how about renaming it to kvm_has_x2apic() and making it so that
the second and following invocations wouldn't uselessly call
kvm_vm_enable_cap()

> +{
> +return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
> +KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
> +}
> +
>  static int kvm_get_tsc(CPUState *cs)
>  {
>  X86CPU *cpu = X86_CPU(cs);
> diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
> index 36407e0a5dc7..5c369b1c5b40 100644
> --- a/target-i386/kvm_i386.h
> +++ b/target-i386/kvm_i386.h
> @@ -43,4 +43,5 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
>  
>  void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
>  
> +bool kvm_enable_x2apic(void);
>  #endif

Re: [Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

2016-09-29 Thread Paolo Bonzini 


On 29/09/2016 13:23, Radim Krčmář wrote:
> Cluster x2APIC cannot work without KVM's x2apic API when the maximal
> APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
> forbid other APICs and also the old KVM case with less than 9, to
> simplify the code.
> 
> There is no point in enabling EIM in forbidden APICs, so we keep it
> enabled only for the KVM APIC;  unconditionally, because making the
> option depend on KVM version would be a maintanance burden.
> 
> Signed-off-by: Radim Krčmář 
> ---
> v2:
>  * adapt to new intr_eim parameter
>  * provide first linux version that has x2apic api
>  * disable QEMU's LAPIC
> ---
>  hw/i386/intel_iommu.c  | 16 +++-
>  target-i386/kvm-stub.c |  5 +
>  target-i386/kvm.c  | 13 +
>  target-i386/kvm_i386.h |  1 +
>  4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 47141cea64f4..b1afe5b133e0 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -32,6 +32,7 @@
>  #include "hw/pci-host/q35.h"
>  #include "sysemu/kvm.h"
>  #include "hw/i386/apic_internal.h"
> +#include "kvm_i386.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>  s->intr_eim = ON_OFF_AUTO_OFF;
>  }
>  if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> -s->intr_eim = ON_OFF_AUTO_ON;
> +s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
> +  : ON_OFF_AUTO_OFF;
> +}
> +if (s->intr_eim == ON_OFF_AUTO_ON) {
> +if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
> +error_report("intel-iommu,eim=on requires support on the KVM 
> side "
> + "(X2APIC_API, first shipped in v4.7).");
> +exit(1);

Please use error_setg and return instead (same in patches 4 and 5).

Paolo

> +}
> +if (!kvm_irqchip_in_kernel()) {
> +error_report("intel-iommu,eim=on requires "
> + "accel=kvm,kernel-irqchip=split.");
> +exit(1);
> +}
>  }
>  
>  memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
> diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
> index cdf15061091d..bda4dc2f0c57 100644
> --- a/target-i386/kvm-stub.c
> +++ b/target-i386/kvm-stub.c
> @@ -25,6 +25,11 @@ bool kvm_has_smm(void)
>  return 1;
>  }
>  
> +bool kvm_enable_x2apic(void)
> +{
> +return false;
> +}
> +
>  /* This function is only called inside conditionals which we
>   * rely on the compiler to optimize out when CONFIG_KVM is not
>   * defined.
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ee1f53e56953..0fd664648665 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -122,6 +122,19 @@ bool kvm_allows_irq0_override(void)
>  return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
>  }
>  
> +static bool kvm_x2apic_api_set_flags(uint64_t flags)
> +{
> +KVMState *s = KVM_STATE(current_machine->accelerator);
> +
> +return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
> +}
> +
> +bool kvm_enable_x2apic(void)
> +{
> +return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
> +KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
> +}
> +
>  static int kvm_get_tsc(CPUState *cs)
>  {
>  X86CPU *cpu = X86_CPU(cs);
> diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
> index 36407e0a5dc7..5c369b1c5b40 100644
> --- a/target-i386/kvm_i386.h
> +++ b/target-i386/kvm_i386.h
> @@ -43,4 +43,5 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
>  
>  void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
>  
> +bool kvm_enable_x2apic(void);
>  #endif
>

[Qemu-devel] [PATCH v2 6/7] intel_iommu: reject broken EIM

2016-09-29 Thread Radim Krčmář 
Cluster x2APIC cannot work without KVM's x2apic API when the maximal
APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
forbid other APICs and also the old KVM case with less than 9, to
simplify the code.

There is no point in enabling EIM in forbidden APICs, so we keep it
enabled only for the KVM APIC;  unconditionally, because making the
option depend on KVM version would be a maintanance burden.

Signed-off-by: Radim Krčmář 
---
v2:
 * adapt to new intr_eim parameter
 * provide first linux version that has x2apic api
 * disable QEMU's LAPIC
---
 hw/i386/intel_iommu.c  | 16 +++-
 target-i386/kvm-stub.c |  5 +
 target-i386/kvm.c  | 13 +
 target-i386/kvm_i386.h |  1 +
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 47141cea64f4..b1afe5b133e0 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -32,6 +32,7 @@
 #include "hw/pci-host/q35.h"
 #include "sysemu/kvm.h"
 #include "hw/i386/apic_internal.h"
+#include "kvm_i386.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -2481,7 +2482,20 @@ static void vtd_realize(DeviceState *dev, Error **errp)
 s->intr_eim = ON_OFF_AUTO_OFF;
 }
 if (s->intr_eim == ON_OFF_AUTO_AUTO) {
-s->intr_eim = ON_OFF_AUTO_ON;
+s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON
+  : ON_OFF_AUTO_OFF;
+}
+if (s->intr_eim == ON_OFF_AUTO_ON) {
+if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
+error_report("intel-iommu,eim=on requires support on the KVM side "
+ "(X2APIC_API, first shipped in v4.7).");
+exit(1);
+}
+if (!kvm_irqchip_in_kernel()) {
+error_report("intel-iommu,eim=on requires "
+ "accel=kvm,kernel-irqchip=split.");
+exit(1);
+}
 }
 
 memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
index cdf15061091d..bda4dc2f0c57 100644
--- a/target-i386/kvm-stub.c
+++ b/target-i386/kvm-stub.c
@@ -25,6 +25,11 @@ bool kvm_has_smm(void)
 return 1;
 }
 
+bool kvm_enable_x2apic(void)
+{
+return false;
+}
+
 /* This function is only called inside conditionals which we
  * rely on the compiler to optimize out when CONFIG_KVM is not
  * defined.
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ee1f53e56953..0fd664648665 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -122,6 +122,19 @@ bool kvm_allows_irq0_override(void)
 return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
 }
 
+static bool kvm_x2apic_api_set_flags(uint64_t flags)
+{
+KVMState *s = KVM_STATE(current_machine->accelerator);
+
+return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
+}
+
+bool kvm_enable_x2apic(void)
+{
+return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
+KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
+}
+
 static int kvm_get_tsc(CPUState *cs)
 {
 X86CPU *cpu = X86_CPU(cs);
diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
index 36407e0a5dc7..5c369b1c5b40 100644
--- a/target-i386/kvm_i386.h
+++ b/target-i386/kvm_i386.h
@@ -43,4 +43,5 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
 
 void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
 
+bool kvm_enable_x2apic(void);
 #endif
-- 
2.10.0