diff mbox

[v4,2/8] apic: add send_msi() to APICCommonClass

Message ID 20161005130657.3399-3-rkrcmar@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Radim Krčmář Oct. 5, 2016, 1:06 p.m. UTC 
The MMIO based interface to APIC doesn't work well with MSIs that have
upper address bits set (remapped x2APIC MSIs).  A specialized interface
is a quick and dirty way to avoid the shortcoming.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4: r-b Igor
v2: change apic_send_msi() to accept MSIMessage [Igor]
---
 hw/i386/kvm/apic.c              | 19 +++++++++++++------
 hw/i386/xen/xen_apic.c          |  6 ++++++
 hw/intc/apic.c                  |  8 ++++++--
 include/hw/i386/apic_internal.h |  4 ++++
 4 files changed, 29 insertions(+), 8 deletions(-)

Comments

Peter Xu Oct. 8, 2016, 6:37 a.m. UTC | #1 
On Wed, Oct 05, 2016 at 03:06:51PM +0200, Radim Krčmář wrote:
> The MMIO based interface to APIC doesn't work well with MSIs that have
> upper address bits set (remapped x2APIC MSIs).  A specialized interface
> is a quick and dirty way to avoid the shortcoming.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

And...

> ---
> v4: r-b Igor
> v2: change apic_send_msi() to accept MSIMessage [Igor]
> ---
>  hw/i386/kvm/apic.c              | 19 +++++++++++++------
>  hw/i386/xen/xen_apic.c          |  6 ++++++
>  hw/intc/apic.c                  |  8 ++++++--
>  include/hw/i386/apic_internal.h |  4 ++++
>  4 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index c016e63fc2ba..be55102c00ca 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -169,6 +169,17 @@ static void kvm_apic_external_nmi(APICCommonState *s)
>      run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s);
>  }
>  
> +static void kvm_send_msi(MSIMessage *msg)
> +{
> +    int ret;
> +
> +    ret = kvm_irqchip_send_msi(kvm_state, *msg);
> +    if (ret < 0) {
> +        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
> +                strerror(-ret));

Maybe use error_report() better? A nit not sufficient for a new spin
though.

And, this patch is assuming MSIMessage as host endianess (which is
good to me). Not sure whether we need fixes for the whole MSIMessage
cleanup (after all, kvm_irqchip_send_msi() is taking it as LE). Or we
can do it afterwards since it won't break anything AFAIU.

-- peterx
Michael S. Tsirkin Oct. 9, 2016, 8:54 p.m. UTC | #2 
On Sat, Oct 08, 2016 at 02:37:59PM +0800, Peter Xu wrote:
> On Wed, Oct 05, 2016 at 03:06:51PM +0200, Radim Krčmář wrote:
> > The MMIO based interface to APIC doesn't work well with MSIs that have
> > upper address bits set (remapped x2APIC MSIs).  A specialized interface
> > is a quick and dirty way to avoid the shortcoming.
> > 
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> And...
> 
> > ---
> > v4: r-b Igor
> > v2: change apic_send_msi() to accept MSIMessage [Igor]
> > ---
> >  hw/i386/kvm/apic.c              | 19 +++++++++++++------
> >  hw/i386/xen/xen_apic.c          |  6 ++++++
> >  hw/intc/apic.c                  |  8 ++++++--
> >  include/hw/i386/apic_internal.h |  4 ++++
> >  4 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> > index c016e63fc2ba..be55102c00ca 100644
> > --- a/hw/i386/kvm/apic.c
> > +++ b/hw/i386/kvm/apic.c
> > @@ -169,6 +169,17 @@ static void kvm_apic_external_nmi(APICCommonState *s)
> >      run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s);
> >  }
> >  
> > +static void kvm_send_msi(MSIMessage *msg)
> > +{
> > +    int ret;
> > +
> > +    ret = kvm_irqchip_send_msi(kvm_state, *msg);
> > +    if (ret < 0) {
> > +        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
> > +                strerror(-ret));
> 
> Maybe use error_report() better? A nit not sufficient for a new spin
> though.
> 
> And, this patch is assuming MSIMessage as host endianess (which is
> good to me). Not sure whether we need fixes for the whole MSIMessage
> cleanup (after all, kvm_irqchip_send_msi() is taking it as LE). Or we
> can do it afterwards since it won't break anything AFAIU.
> 
> -- peterx

I think this is a bug really. Paolo?

We really need to start annotating le fields e.g. the way
Linux does this, to allow static checks with sparse.
Radim Krčmář Oct. 10, 2016, 1:35 p.m. UTC | #3 
2016-10-08 14:37+0800, Peter Xu:
> On Wed, Oct 05, 2016 at 03:06:51PM +0200, Radim Krčmář wrote:
>> The MMIO based interface to APIC doesn't work well with MSIs that have
>> upper address bits set (remapped x2APIC MSIs).  A specialized interface
>> is a quick and dirty way to avoid the shortcoming.
>> 
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks.

>> ---
>> v4: r-b Igor
>> v2: change apic_send_msi() to accept MSIMessage [Igor]
>> ---
>>  hw/i386/kvm/apic.c              | 19 +++++++++++++------
>>  hw/i386/xen/xen_apic.c          |  6 ++++++
>>  hw/intc/apic.c                  |  8 ++++++--
>>  include/hw/i386/apic_internal.h |  4 ++++
>>  4 files changed, 29 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
>> index c016e63fc2ba..be55102c00ca 100644
>> --- a/hw/i386/kvm/apic.c
>> +++ b/hw/i386/kvm/apic.c
>> @@ -169,6 +169,17 @@ static void kvm_apic_external_nmi(APICCommonState *s)
>>      run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s);
>>  }
>>  
>> +static void kvm_send_msi(MSIMessage *msg)
>> +{
>> +    int ret;
>> +
>> +    ret = kvm_irqchip_send_msi(kvm_state, *msg);
>> +    if (ret < 0) {
>> +        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
>> +                strerror(-ret));
> 
> Maybe use error_report() better? A nit not sufficient for a new spin
> though.

Yes, but it is a separate logical change, so I'd do that in a separate
cleaup patch that amends it in the whole file ...
(And I prefer to copy-paste code verbatim.)

> And, this patch is assuming MSIMessage as host endianess (which is
> good to me). Not sure whether we need fixes for the whole MSIMessage
> cleanup (after all, kvm_irqchip_send_msi() is taking it as LE). Or we
> can do it afterwards since it won't break anything AFAIU.

This patch doesn't affect existing callers, and IR known to be broken,
so I think we can sort endianess out later ...
Peter Xu Oct. 10, 2016, 11:50 p.m. UTC | #4 
On Mon, Oct 10, 2016 at 03:35:26PM +0200, Radim Krčmář wrote:

[...]

> Yes, but it is a separate logical change, so I'd do that in a separate
> cleaup patch that amends it in the whole file ...
> (And I prefer to copy-paste code verbatim.)

Sure.

> 
> > And, this patch is assuming MSIMessage as host endianess (which is
> > good to me). Not sure whether we need fixes for the whole MSIMessage
> > cleanup (after all, kvm_irqchip_send_msi() is taking it as LE). Or we
> > can do it afterwards since it won't break anything AFAIU.
> 
> This patch doesn't affect existing callers, and IR known to be broken,
> so I think we can sort endianess out later ...

Agree. Patch:

  [PATCH] Revert "KVM: MSI: Swap payload to native endianness"

is for the cleanup actually. Though I am not sure whether that's
enough and correct. Looking forward to any review comments.

Thanks,

-- peterx
diff mbox

Patch

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index c016e63fc2ba..be55102c00ca 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -169,6 +169,17 @@  static void kvm_apic_external_nmi(APICCommonState *s)
     run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s);
 }
 
+static void kvm_send_msi(MSIMessage *msg)
+{
+    int ret;
+
+    ret = kvm_irqchip_send_msi(kvm_state, *msg);
+    if (ret < 0) {
+        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
+                strerror(-ret));
+    }
+}
+
 static uint64_t kvm_apic_mem_read(void *opaque, hwaddr addr,
                                   unsigned size)
 {
@@ -179,13 +190,8 @@  static void kvm_apic_mem_write(void *opaque, hwaddr addr,
                                uint64_t data, unsigned size)
 {
     MSIMessage msg = { .address = addr, .data = data };
-    int ret;
 
-    ret = kvm_irqchip_send_msi(kvm_state, msg);
-    if (ret < 0) {
-        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
-                strerror(-ret));
-    }
+    kvm_send_msi(&msg);
 }
 
 static const MemoryRegionOps kvm_apic_io_ops = {
@@ -232,6 +238,7 @@  static void kvm_apic_class_init(ObjectClass *klass, void *data)
     k->enable_tpr_reporting = kvm_apic_enable_tpr_reporting;
     k->vapic_base_update = kvm_apic_vapic_base_update;
     k->external_nmi = kvm_apic_external_nmi;
+    k->send_msi = kvm_send_msi;
 }
 
 static const TypeInfo kvm_apic_info = {
diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c
index 21d68ee04b0a..55769eba7ede 100644
--- a/hw/i386/xen/xen_apic.c
+++ b/hw/i386/xen/xen_apic.c
@@ -68,6 +68,11 @@  static void xen_apic_external_nmi(APICCommonState *s)
 {
 }
 
+static void xen_send_msi(MSIMessage *msi)
+{
+    xen_hvm_inject_msi(msi->address, msi->data);
+}
+
 static void xen_apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
@@ -78,6 +83,7 @@  static void xen_apic_class_init(ObjectClass *klass, void *data)
     k->get_tpr = xen_apic_get_tpr;
     k->vapic_base_update = xen_apic_vapic_base_update;
     k->external_nmi = xen_apic_external_nmi;
+    k->send_msi = xen_send_msi;
 }
 
 static const TypeInfo xen_apic_info = {
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 7bd1d279c463..fe15fb602473 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -740,8 +740,10 @@  static uint32_t apic_mem_readl(void *opaque, hwaddr addr)
     return val;
 }
 
-static void apic_send_msi(hwaddr addr, uint32_t data)
+static void apic_send_msi(MSIMessage *msi)
 {
+    uint64_t addr = msi->address;
+    uint32_t data = msi->data;
     uint8_t dest = (addr & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
     uint8_t vector = (data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT;
     uint8_t dest_mode = (addr >> MSI_ADDR_DEST_MODE_SHIFT) & 0x1;
@@ -762,7 +764,8 @@  static void apic_mem_writel(void *opaque, hwaddr addr, uint32_t val)
          * APIC is connected directly to the CPU.
          * Mapping them on the global bus happens to work because
          * MSI registers are reserved in APIC MMIO and vice versa. */
-        apic_send_msi(addr, val);
+        MSIMessage msi = { .address = addr, .data = val };
+        apic_send_msi(&msi);
         return;
     }
 
@@ -913,6 +916,7 @@  static void apic_class_init(ObjectClass *klass, void *data)
     k->external_nmi = apic_external_nmi;
     k->pre_save = apic_pre_save;
     k->post_load = apic_post_load;
+    k->send_msi = apic_send_msi;
 }
 
 static const TypeInfo apic_info = {
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 286684857e9f..cdd11fb0938f 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -146,6 +146,10 @@  typedef struct APICCommonClass
     void (*pre_save)(APICCommonState *s);
     void (*post_load)(APICCommonState *s);
     void (*reset)(APICCommonState *s);
+    /* send_msi emulates an APIC bus and its proper place would be in a new
+     * device, but it's convenient to have it here for now.
+     */
+    void (*send_msi)(MSIMessage *msi);
 } APICCommonClass;
 
 struct APICCommonState {