[RFC PATCH] x86: Add a new physdev_op PHYSDEVOP_nr_irqs_gsi

[RFC PATCH] x86: Add a new physdev_op PHYSDEVOP_nr_irqs_gsi

[Lin Ming]

This new physdev_op is added for Linux guest kernel to get the correct
nr_irqs_gsi value.

See below Linux kernel patch for detail explanation.

[RFC PATCH] xen: get correct nr_irqs_gsi value from hypervisor
http://marc.info/?l=xen-devel&m=133407004503365&w=2

Signed-off-by: Lin Ming <mlin@ss.pku.edu.cn>
---
 xen/arch/x86/physdev.c       |    8 ++++++++
 xen/include/public/physdev.h |    6 ++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 05fff9e..0912db0 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -688,6 +688,14 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
                               setup_gsi.polarity);
         break; 
     }
+    case PHYSDEVOP_nr_irqs_gsi: {
+        struct physdev_nr_irqs_gsi out;
+
+        out.nr_irqs_gsi = nr_irqs_gsi;
+        ret = copy_to_guest(arg, &out, 1) ? -EFAULT : 0;
+
+        break;
+    }
     case PHYSDEVOP_get_free_pirq: {
         struct physdev_get_free_pirq out;
         struct domain *d;
diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
index b78eeba..7856fc2 100644
--- a/xen/include/public/physdev.h
+++ b/xen/include/public/physdev.h
@@ -312,6 +312,12 @@ struct physdev_pci_device {
 typedef struct physdev_pci_device physdev_pci_device_t;
 DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t);
 
+#define PHYSDEVOP_nr_irqs_gsi           29
+struct physdev_nr_irqs_gsi {
+    /* OUT */
+    uint32_t nr_irqs_gsi;
+};
+
 /*
  * Notify that some PIRQ-bound event channels have been unmasked.
  * ** This command is obsolete since interface version 0x00030202 and is **
-- 
1.7.2.5

Re: [RFC PATCH] x86: Add a new physdev_op PHYSDEVOP_nr_irqs_gsi

[Jan Beulich]

>>> On 10.04.12 at 17:13, Lin Ming <mlin@ss.pku.edu.cn> wrote:
> This new physdev_op is added for Linux guest kernel to get the correct
> nr_irqs_gsi value.

I'm not convinced this is really needed - the kernel can work out the
right number without any hypercall afaict.

Jan

> See below Linux kernel patch for detail explanation.
> 
> [RFC PATCH] xen: get correct nr_irqs_gsi value from hypervisor
> http://marc.info/?l=xen-devel&m=133407004503365&w=2 
> 
> Signed-off-by: Lin Ming <mlin@ss.pku.edu.cn>
> ---
>  xen/arch/x86/physdev.c       |    8 ++++++++
>  xen/include/public/physdev.h |    6 ++++++
>  2 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> index 05fff9e..0912db0 100644
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -688,6 +688,14 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>                                setup_gsi.polarity);
>          break; 
>      }
> +    case PHYSDEVOP_nr_irqs_gsi: {
> +        struct physdev_nr_irqs_gsi out;
> +
> +        out.nr_irqs_gsi = nr_irqs_gsi;
> +        ret = copy_to_guest(arg, &out, 1) ? -EFAULT : 0;
> +
> +        break;
> +    }
>      case PHYSDEVOP_get_free_pirq: {
>          struct physdev_get_free_pirq out;
>          struct domain *d;
> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
> index b78eeba..7856fc2 100644
> --- a/xen/include/public/physdev.h
> +++ b/xen/include/public/physdev.h
> @@ -312,6 +312,12 @@ struct physdev_pci_device {
>  typedef struct physdev_pci_device physdev_pci_device_t;
>  DEFINE_XEN_GUEST_HANDLE(physdev_pci_device_t);
>  
> +#define PHYSDEVOP_nr_irqs_gsi           29
> +struct physdev_nr_irqs_gsi {
> +    /* OUT */
> +    uint32_t nr_irqs_gsi;
> +};
> +
>  /*
>   * Notify that some PIRQ-bound event channels have been unmasked.
>   * ** This command is obsolete since interface version 0x00030202 and is **
> -- 
> 1.7.2.5
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [RFC PATCH] x86: Add a new physdev_op PHYSDEVOP_nr_irqs_gsi

[Lin Ming]

On Tue, 2012-04-10 at 16:33 +0100, Jan Beulich wrote:
> >>> On 10.04.12 at 17:13, Lin Ming <mlin@ss.pku.edu.cn> wrote:
> > This new physdev_op is added for Linux guest kernel to get the correct
> > nr_irqs_gsi value.
> 
> I'm not convinced this is really needed - the kernel can work out the
> right number without any hypercall afaict.

In Linux kernel:

mp_register_ioapic(...):

        entries = io_apic_get_redir_entries(idx);
        gsi_cfg = mp_ioapic_gsi_routing(idx);
        gsi_cfg->gsi_base = gsi_base;
        gsi_cfg->gsi_end = gsi_base + entries - 1;

        /*
         * The number of IO-APIC IRQ registers (== #pins):
         */
        ioapics[idx].nr_registers = entries;

        if (gsi_cfg->gsi_end >= gsi_top)
                gsi_top = gsi_cfg->gsi_end + 1;

io_apic_get_redir_entries calls io_apic_read(), which returns wrong
value(0xFFFFFFFF) on Xen Dom0 kernel.

How can we get the correct gsi_top value, which is used to set
nr_irqs_gsi, without hypercall?

The problem here is we don't have a Xen version of io_apic_read in Linux
kernel.

Thanks,
Lin Ming

> 
> Jan

Re: [RFC PATCH] x86: Add a new physdev_op PHYSDEVOP_nr_irqs_gsi

[Jan Beulich]

>>> On 10.04.12 at 17:50, Lin Ming <mlin@ss.pku.edu.cn> wrote:
> On Tue, 2012-04-10 at 16:33 +0100, Jan Beulich wrote:
>> >>> On 10.04.12 at 17:13, Lin Ming <mlin@ss.pku.edu.cn> wrote:
>> > This new physdev_op is added for Linux guest kernel to get the correct
>> > nr_irqs_gsi value.
>> 
>> I'm not convinced this is really needed - the kernel can work out the
>> right number without any hypercall afaict.
> 
> In Linux kernel:
> 
> mp_register_ioapic(...):
> 
>         entries = io_apic_get_redir_entries(idx);
>         gsi_cfg = mp_ioapic_gsi_routing(idx);
>         gsi_cfg->gsi_base = gsi_base;
>         gsi_cfg->gsi_end = gsi_base + entries - 1;
> 
>         /*
>          * The number of IO-APIC IRQ registers (== #pins):
>          */
>         ioapics[idx].nr_registers = entries;
> 
>         if (gsi_cfg->gsi_end >= gsi_top)
>                 gsi_top = gsi_cfg->gsi_end + 1;
> 
> io_apic_get_redir_entries calls io_apic_read(), which returns wrong
> value(0xFFFFFFFF) on Xen Dom0 kernel.

I understand all of the above.

> How can we get the correct gsi_top value, which is used to set
> nr_irqs_gsi, without hypercall?
> 
> The problem here is we don't have a Xen version of io_apic_read in Linux
> kernel.

Meaning you need to craft one if you need one, the more that
struct io_apic_ops already exists. The traditional kernel was able
to do so quite fine, and hence didn't need any hypercall.

Jan

Re: [RFC PATCH] x86: Add a new physdev_op PHYSDEVOP_nr_irqs_gsi

[Stefano Stabellini]

On Tue, 10 Apr 2012, Lin Ming wrote:
> On Tue, 2012-04-10 at 16:33 +0100, Jan Beulich wrote:
> > >>> On 10.04.12 at 17:13, Lin Ming <mlin@ss.pku.edu.cn> wrote:
> > > This new physdev_op is added for Linux guest kernel to get the correct
> > > nr_irqs_gsi value.
> > 
> > I'm not convinced this is really needed - the kernel can work out the
> > right number without any hypercall afaict.
> 
> In Linux kernel:
> 
> mp_register_ioapic(...):
> 
>         entries = io_apic_get_redir_entries(idx);
>         gsi_cfg = mp_ioapic_gsi_routing(idx);
>         gsi_cfg->gsi_base = gsi_base;
>         gsi_cfg->gsi_end = gsi_base + entries - 1;
> 
>         /*
>          * The number of IO-APIC IRQ registers (== #pins):
>          */
>         ioapics[idx].nr_registers = entries;
> 
>         if (gsi_cfg->gsi_end >= gsi_top)
>                 gsi_top = gsi_cfg->gsi_end + 1;
> 
> io_apic_get_redir_entries calls io_apic_read(), which returns wrong
> value(0xFFFFFFFF) on Xen Dom0 kernel.
> 
> How can we get the correct gsi_top value, which is used to set
> nr_irqs_gsi, without hypercall?
> 
> The problem here is we don't have a Xen version of io_apic_read in Linux
> kernel.

Actually we do: http://marc.info/?l=linux-kernel&m=133295662314519

Re: [RFC PATCH] x86: Add a new physdev_op PHYSDEVOP_nr_irqs_gsi

[Zhang, Xiantao]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Wednesday, April 11, 2012 12:21 AM
> To: Lin Ming
> Cc: Jan Beulich; Konrad Rzeszutek Wilk; Zhang, Xiantao; xen-devel
> Subject: Re: [Xen-devel] [RFC PATCH] x86: Add a new physdev_op
> PHYSDEVOP_nr_irqs_gsi
> 
> On Tue, 10 Apr 2012, Lin Ming wrote:
> > On Tue, 2012-04-10 at 16:33 +0100, Jan Beulich wrote:
> > > >>> On 10.04.12 at 17:13, Lin Ming <mlin@ss.pku.edu.cn> wrote:
> > > > This new physdev_op is added for Linux guest kernel to get the
> > > > correct nr_irqs_gsi value.
> > >
> > > I'm not convinced this is really needed - the kernel can work out
> > > the right number without any hypercall afaict.
> >
> > In Linux kernel:
> >
> > mp_register_ioapic(...):
> >
> >         entries = io_apic_get_redir_entries(idx);
> >         gsi_cfg = mp_ioapic_gsi_routing(idx);
> >         gsi_cfg->gsi_base = gsi_base;
> >         gsi_cfg->gsi_end = gsi_base + entries - 1;
> >
> >         /*
> >          * The number of IO-APIC IRQ registers (== #pins):
> >          */
> >         ioapics[idx].nr_registers = entries;
> >
> >         if (gsi_cfg->gsi_end >= gsi_top)
> >                 gsi_top = gsi_cfg->gsi_end + 1;
> >
> > io_apic_get_redir_entries calls io_apic_read(), which returns wrong
> > value(0xFFFFFFFF) on Xen Dom0 kernel.
> >
> > How can we get the correct gsi_top value, which is used to set
> > nr_irqs_gsi, without hypercall?
> >
> > The problem here is we don't have a Xen version of io_apic_read in
> > Linux kernel.
> 
> Actually we do: http://marc.info/?l=linux-kernel&m=133295662314519

This is not enough, seems fixed value are returned for each read ?
Xiantao

Re: [RFC PATCH] x86: Add a new physdev_op PHYSDEVOP_nr_irqs_gsi

[Stefano Stabellini]

On Wed, 11 Apr 2012, Zhang, Xiantao wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: Wednesday, April 11, 2012 12:21 AM
> > To: Lin Ming
> > Cc: Jan Beulich; Konrad Rzeszutek Wilk; Zhang, Xiantao; xen-devel
> > Subject: Re: [Xen-devel] [RFC PATCH] x86: Add a new physdev_op
> > PHYSDEVOP_nr_irqs_gsi
> > 
> > On Tue, 10 Apr 2012, Lin Ming wrote:
> > > On Tue, 2012-04-10 at 16:33 +0100, Jan Beulich wrote:
> > > > >>> On 10.04.12 at 17:13, Lin Ming <mlin@ss.pku.edu.cn> wrote:
> > > > > This new physdev_op is added for Linux guest kernel to get the
> > > > > correct nr_irqs_gsi value.
> > > >
> > > > I'm not convinced this is really needed - the kernel can work out
> > > > the right number without any hypercall afaict.
> > >
> > > In Linux kernel:
> > >
> > > mp_register_ioapic(...):
> > >
> > >         entries = io_apic_get_redir_entries(idx);
> > >         gsi_cfg = mp_ioapic_gsi_routing(idx);
> > >         gsi_cfg->gsi_base = gsi_base;
> > >         gsi_cfg->gsi_end = gsi_base + entries - 1;
> > >
> > >         /*
> > >          * The number of IO-APIC IRQ registers (== #pins):
> > >          */
> > >         ioapics[idx].nr_registers = entries;
> > >
> > >         if (gsi_cfg->gsi_end >= gsi_top)
> > >                 gsi_top = gsi_cfg->gsi_end + 1;
> > >
> > > io_apic_get_redir_entries calls io_apic_read(), which returns wrong
> > > value(0xFFFFFFFF) on Xen Dom0 kernel.
> > >
> > > How can we get the correct gsi_top value, which is used to set
> > > nr_irqs_gsi, without hypercall?
> > >
> > > The problem here is we don't have a Xen version of io_apic_read in
> > > Linux kernel.
> > 
> > Actually we do: http://marc.info/?l=linux-kernel&m=133295662314519
> 
> This is not enough, seems fixed value are returned for each read ?

Yes, it looks like we are always returning 0x00170020, that means 24
entries that I guess is correct for the vast majority of io_apics.

We could limit ourselves to adding a comment saying "we assume the
io_apic has 24 redir entries".

Or we could use PHYSDEVOP_apic_read to do an actual io_apic read for
reg 0x1.

Re: [RFC PATCH] x86: Add a new physdev_op PHYSDEVOP_nr_irqs_gsi

[Ian Campbell]

On Wed, 2012-04-11 at 12:42 +0100, Stefano Stabellini wrote:
> On Wed, 11 Apr 2012, Zhang, Xiantao wrote:
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > > Sent: Wednesday, April 11, 2012 12:21 AM
> > > To: Lin Ming
> > > Cc: Jan Beulich; Konrad Rzeszutek Wilk; Zhang, Xiantao; xen-devel
> > > Subject: Re: [Xen-devel] [RFC PATCH] x86: Add a new physdev_op
> > > PHYSDEVOP_nr_irqs_gsi
> > > 
> > > On Tue, 10 Apr 2012, Lin Ming wrote:
> > > > On Tue, 2012-04-10 at 16:33 +0100, Jan Beulich wrote:
> > > > > >>> On 10.04.12 at 17:13, Lin Ming <mlin@ss.pku.edu.cn> wrote:
> > > > > > This new physdev_op is added for Linux guest kernel to get the
> > > > > > correct nr_irqs_gsi value.
> > > > >
> > > > > I'm not convinced this is really needed - the kernel can work out
> > > > > the right number without any hypercall afaict.
> > > >
> > > > In Linux kernel:
> > > >
> > > > mp_register_ioapic(...):
> > > >
> > > >         entries = io_apic_get_redir_entries(idx);
> > > >         gsi_cfg = mp_ioapic_gsi_routing(idx);
> > > >         gsi_cfg->gsi_base = gsi_base;
> > > >         gsi_cfg->gsi_end = gsi_base + entries - 1;
> > > >
> > > >         /*
> > > >          * The number of IO-APIC IRQ registers (== #pins):
> > > >          */
> > > >         ioapics[idx].nr_registers = entries;
> > > >
> > > >         if (gsi_cfg->gsi_end >= gsi_top)
> > > >                 gsi_top = gsi_cfg->gsi_end + 1;
> > > >
> > > > io_apic_get_redir_entries calls io_apic_read(), which returns wrong
> > > > value(0xFFFFFFFF) on Xen Dom0 kernel.
> > > >
> > > > How can we get the correct gsi_top value, which is used to set
> > > > nr_irqs_gsi, without hypercall?
> > > >
> > > > The problem here is we don't have a Xen version of io_apic_read in
> > > > Linux kernel.
> > > 
> > > Actually we do: http://marc.info/?l=linux-kernel&m=133295662314519

Why was this patch never seen on xen-devel?
 
> > This is not enough, seems fixed value are returned for each read ?
> 
> Yes, it looks like we are always returning 0x00170020, that means 24
> entries that I guess is correct for the vast majority of io_apics.
> 
> We could limit ourselves to adding a comment saying "we assume the
> io_apic has 24 redir entries".
> 
> Or we could use PHYSDEVOP_apic_read to do an actual io_apic read for
> reg 0x1.

This seems like the obviously right thing to do, compared with
hardcoding 24...

Ian.

Re: [RFC PATCH] x86: Add a new physdev_op PHYSDEVOP_nr_irqs_gsi

[Konrad Rzeszutek Wilk]

On Wed, Apr 11, 2012 at 12:49:12PM +0100, Ian Campbell wrote:
> On Wed, 2012-04-11 at 12:42 +0100, Stefano Stabellini wrote:
> > On Wed, 11 Apr 2012, Zhang, Xiantao wrote:
> > > > -----Original Message-----
> > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > > > Sent: Wednesday, April 11, 2012 12:21 AM
> > > > To: Lin Ming
> > > > Cc: Jan Beulich; Konrad Rzeszutek Wilk; Zhang, Xiantao; xen-devel
> > > > Subject: Re: [Xen-devel] [RFC PATCH] x86: Add a new physdev_op
> > > > PHYSDEVOP_nr_irqs_gsi
> > > > 
> > > > On Tue, 10 Apr 2012, Lin Ming wrote:
> > > > > On Tue, 2012-04-10 at 16:33 +0100, Jan Beulich wrote:
> > > > > > >>> On 10.04.12 at 17:13, Lin Ming <mlin@ss.pku.edu.cn> wrote:
> > > > > > > This new physdev_op is added for Linux guest kernel to get the
> > > > > > > correct nr_irqs_gsi value.
> > > > > >
> > > > > > I'm not convinced this is really needed - the kernel can work out
> > > > > > the right number without any hypercall afaict.
> > > > >
> > > > > In Linux kernel:
> > > > >
> > > > > mp_register_ioapic(...):
> > > > >
> > > > >         entries = io_apic_get_redir_entries(idx);
> > > > >         gsi_cfg = mp_ioapic_gsi_routing(idx);
> > > > >         gsi_cfg->gsi_base = gsi_base;
> > > > >         gsi_cfg->gsi_end = gsi_base + entries - 1;
> > > > >
> > > > >         /*
> > > > >          * The number of IO-APIC IRQ registers (== #pins):
> > > > >          */
> > > > >         ioapics[idx].nr_registers = entries;
> > > > >
> > > > >         if (gsi_cfg->gsi_end >= gsi_top)
> > > > >                 gsi_top = gsi_cfg->gsi_end + 1;
> > > > >
> > > > > io_apic_get_redir_entries calls io_apic_read(), which returns wrong
> > > > > value(0xFFFFFFFF) on Xen Dom0 kernel.$

It is actually now 0xfd.

> > > > >
> > > > > How can we get the correct gsi_top value, which is used to set
> > > > > nr_irqs_gsi, without hypercall?

So, why is this important?

> > > > >
> > > > > The problem here is we don't have a Xen version of io_apic_read in
> > > > > Linux kernel.
> > > > 
> > > > Actually we do: http://marc.info/?l=linux-kernel&m=133295662314519
> 
> Why was this patch never seen on xen-devel?

Probably b/c I forgot to put the CC on it. Either way it first
has to go through Ingo's tree for 3.5.

>  
> > > This is not enough, seems fixed value are returned for each read ?
> > 
> > Yes, it looks like we are always returning 0x00170020, that means 24
> > entries that I guess is correct for the vast majority of io_apics.
> > 
> > We could limit ourselves to adding a comment saying "we assume the
> > io_apic has 24 redir entries".
> > 
> > Or we could use PHYSDEVOP_apic_read to do an actual io_apic read for
> > reg 0x1.
> 
> This seems like the obviously right thing to do, compared with
> hardcoding 24...

Could do as well.
> 
> Ian.
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel