| Message ID | 20170407173307.9788-20-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 04/07/2017 06:32 PM, Andre Przywara wrote: > The target CPU for an LPI is encoded in the interrupt translation table > entry, so can't be easily derived from just an LPI number (short of > walking *all* tables and find the matching LPI). > To avoid this in case we need to know the VCPU (for the INVALL command, > for instance), put the VCPU ID in the struct pending_irq, so that it is > easily accessible. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/include/asm-arm/vgic.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index 86a1a89..9145b42 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -88,6 +88,9 @@ struct pending_irq > * TODO: when implementing irq migration, taking only the current > * vgic lock is not going to be enough. */ > struct list_head lr_queue; > +#ifdef CONFIG_HAS_ITS > + uint16_t lpi_vcpu_id; /* The VCPU for an LPI. */ > +#endif I am sorry, but in the current state this is a nack from me side. The pending_irq structure will increase by 8 bytes because of the alignment. This will affect all the irq_pending as soon as CONFIG_HAS_ITS is enabled even if the platform is not using ITS. Looking at the current usage it is only used by INVALL, which lead to confirm my first nack. Please either find a hole in pending_irq or find an alternative solution. Cheers,
On Fri, 7 Apr 2017, Julien Grall wrote: > Hi Andre, > > On 04/07/2017 06:32 PM, Andre Przywara wrote: > > The target CPU for an LPI is encoded in the interrupt translation table > > entry, so can't be easily derived from just an LPI number (short of > > walking *all* tables and find the matching LPI). > > To avoid this in case we need to know the VCPU (for the INVALL command, > > for instance), put the VCPU ID in the struct pending_irq, so that it is > > easily accessible. > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > xen/include/asm-arm/vgic.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > > index 86a1a89..9145b42 100644 > > --- a/xen/include/asm-arm/vgic.h > > +++ b/xen/include/asm-arm/vgic.h > > @@ -88,6 +88,9 @@ struct pending_irq > > * TODO: when implementing irq migration, taking only the current > > * vgic lock is not going to be enough. */ > > struct list_head lr_queue; > > +#ifdef CONFIG_HAS_ITS > > + uint16_t lpi_vcpu_id; /* The VCPU for an LPI. */ > > +#endif > > I am sorry, but in the current state this is a nack from me side. The > pending_irq structure will increase by 8 bytes because of the alignment. > > This will affect all the irq_pending as soon as CONFIG_HAS_ITS is enabled even > if the platform is not using ITS. > > Looking at the current usage it is only used by INVALL, which lead to confirm > my first nack. > > Please either find a hole in pending_irq or find an alternative solution. Yeah, I was thinking the same thing. That is why I suggested the #ifdef CONFIG_HAS_ITS. At least the problem would remain confined to ITS. I recognize that we need a better solution, but I would be willing to take this as-is with the ifdef for 4.9, assuming that everything else is in order.
Hi Stefano, On 04/07/2017 11:14 PM, Stefano Stabellini wrote: > On Fri, 7 Apr 2017, Julien Grall wrote: >> Hi Andre, >> >> On 04/07/2017 06:32 PM, Andre Przywara wrote: >>> The target CPU for an LPI is encoded in the interrupt translation table >>> entry, so can't be easily derived from just an LPI number (short of >>> walking *all* tables and find the matching LPI). >>> To avoid this in case we need to know the VCPU (for the INVALL command, >>> for instance), put the VCPU ID in the struct pending_irq, so that it is >>> easily accessible. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> xen/include/asm-arm/vgic.h | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h >>> index 86a1a89..9145b42 100644 >>> --- a/xen/include/asm-arm/vgic.h >>> +++ b/xen/include/asm-arm/vgic.h >>> @@ -88,6 +88,9 @@ struct pending_irq >>> * TODO: when implementing irq migration, taking only the current >>> * vgic lock is not going to be enough. */ >>> struct list_head lr_queue; >>> +#ifdef CONFIG_HAS_ITS >>> + uint16_t lpi_vcpu_id; /* The VCPU for an LPI. */ >>> +#endif >> >> I am sorry, but in the current state this is a nack from me side. The >> pending_irq structure will increase by 8 bytes because of the alignment. >> >> This will affect all the irq_pending as soon as CONFIG_HAS_ITS is enabled even >> if the platform is not using ITS. >> >> Looking at the current usage it is only used by INVALL, which lead to confirm >> my first nack. >> >> Please either find a hole in pending_irq or find an alternative solution. > > Yeah, I was thinking the same thing. That is why I suggested the #ifdef > CONFIG_HAS_ITS. At least the problem would remain confined to ITS. > > I recognize that we need a better solution, but I would be willing to > take this as-is with the ifdef for 4.9, assuming that everything else is > in order. Still, it is 8 bytes per IRQ. Which means increase internal memory usage by up to 8KB (assuming DOM0 with 1024 interrupts). Xen currently support only up to 128 vCPUs and there is a 1 byte hole in the structure. As it is plenty enough, why don't we use this hole? Cheers,
On Fri, 7 Apr 2017, Julien Grall wrote: > Hi Stefano, > > On 04/07/2017 11:14 PM, Stefano Stabellini wrote: > > On Fri, 7 Apr 2017, Julien Grall wrote: > > > Hi Andre, > > > > > > On 04/07/2017 06:32 PM, Andre Przywara wrote: > > > > The target CPU for an LPI is encoded in the interrupt translation table > > > > entry, so can't be easily derived from just an LPI number (short of > > > > walking *all* tables and find the matching LPI). > > > > To avoid this in case we need to know the VCPU (for the INVALL command, > > > > for instance), put the VCPU ID in the struct pending_irq, so that it is > > > > easily accessible. > > > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > --- > > > > xen/include/asm-arm/vgic.h | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > > > > index 86a1a89..9145b42 100644 > > > > --- a/xen/include/asm-arm/vgic.h > > > > +++ b/xen/include/asm-arm/vgic.h > > > > @@ -88,6 +88,9 @@ struct pending_irq > > > > * TODO: when implementing irq migration, taking only the current > > > > * vgic lock is not going to be enough. */ > > > > struct list_head lr_queue; > > > > +#ifdef CONFIG_HAS_ITS > > > > + uint16_t lpi_vcpu_id; /* The VCPU for an LPI. */ > > > > +#endif > > > > > > I am sorry, but in the current state this is a nack from me side. The > > > pending_irq structure will increase by 8 bytes because of the alignment. > > > > > > This will affect all the irq_pending as soon as CONFIG_HAS_ITS is enabled > > > even > > > if the platform is not using ITS. > > > > > > Looking at the current usage it is only used by INVALL, which lead to > > > confirm > > > my first nack. > > > > > > Please either find a hole in pending_irq or find an alternative solution. > > > > Yeah, I was thinking the same thing. That is why I suggested the #ifdef > > CONFIG_HAS_ITS. At least the problem would remain confined to ITS. > > > > I recognize that we need a better solution, but I would be willing to > > take this as-is with the ifdef for 4.9, assuming that everything else is > > in order. > > Still, it is 8 bytes per IRQ. Which means increase internal memory usage by up > to 8KB (assuming DOM0 with 1024 interrupts). > > Xen currently support only up to 128 vCPUs and there is a 1 byte hole in the > structure. As it is plenty enough, why don't we use this hole? Not a bad idea, but it doesn't leave us any room for expansion. Still, it is better than an #ifdef in vgic.h.
Hi Stefano, On 07/04/2017 23:31, Stefano Stabellini wrote: > On Fri, 7 Apr 2017, Julien Grall wrote: >> Hi Stefano, >> >> On 04/07/2017 11:14 PM, Stefano Stabellini wrote: >>> On Fri, 7 Apr 2017, Julien Grall wrote: >>>> Hi Andre, >>>> >>>> On 04/07/2017 06:32 PM, Andre Przywara wrote: >>>>> The target CPU for an LPI is encoded in the interrupt translation table >>>>> entry, so can't be easily derived from just an LPI number (short of >>>>> walking *all* tables and find the matching LPI). >>>>> To avoid this in case we need to know the VCPU (for the INVALL command, >>>>> for instance), put the VCPU ID in the struct pending_irq, so that it is >>>>> easily accessible. >>>>> >>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>>> --- >>>>> xen/include/asm-arm/vgic.h | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h >>>>> index 86a1a89..9145b42 100644 >>>>> --- a/xen/include/asm-arm/vgic.h >>>>> +++ b/xen/include/asm-arm/vgic.h >>>>> @@ -88,6 +88,9 @@ struct pending_irq >>>>> * TODO: when implementing irq migration, taking only the current >>>>> * vgic lock is not going to be enough. */ >>>>> struct list_head lr_queue; >>>>> +#ifdef CONFIG_HAS_ITS >>>>> + uint16_t lpi_vcpu_id; /* The VCPU for an LPI. */ >>>>> +#endif >>>> >>>> I am sorry, but in the current state this is a nack from me side. The >>>> pending_irq structure will increase by 8 bytes because of the alignment. >>>> >>>> This will affect all the irq_pending as soon as CONFIG_HAS_ITS is enabled >>>> even >>>> if the platform is not using ITS. >>>> >>>> Looking at the current usage it is only used by INVALL, which lead to >>>> confirm >>>> my first nack. >>>> >>>> Please either find a hole in pending_irq or find an alternative solution. >>> >>> Yeah, I was thinking the same thing. That is why I suggested the #ifdef >>> CONFIG_HAS_ITS. At least the problem would remain confined to ITS. >>> >>> I recognize that we need a better solution, but I would be willing to >>> take this as-is with the ifdef for 4.9, assuming that everything else is >>> in order. >> >> Still, it is 8 bytes per IRQ. Which means increase internal memory usage by up >> to 8KB (assuming DOM0 with 1024 interrupts). >> >> Xen currently support only up to 128 vCPUs and there is a 1 byte hole in the >> structure. As it is plenty enough, why don't we use this hole? > > Not a bad idea, but it doesn't leave us any room for expansion. Still, > it is better than an #ifdef in vgic.h. I agree that it does not leave any room. But it does not increase the structure by 8 bytes unconditionally (even if it is protected by #ifdef). This would be a call to miss that when we will compile ITS by default and therefore increase pending_irq. But to be fair, I am not a bit fan of re-purposing irq_to_pending for storing LPIs information. This feels like a hack because those value are never used directly by the common code. A better approach would be to introduce a struct pending_lpi which contain irq_to_pending. Something like: struct pending_lpi { struct irq_to_pending irq; uint16_t vcpu_id; uint8_t priority; } Cheers,
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index 86a1a89..9145b42 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -88,6 +88,9 @@ struct pending_irq * TODO: when implementing irq migration, taking only the current * vgic lock is not going to be enough. */ struct list_head lr_queue; +#ifdef CONFIG_HAS_ITS + uint16_t lpi_vcpu_id; /* The VCPU for an LPI. */ +#endif }; #define NR_INTERRUPT_PER_RANK 32
The target CPU for an LPI is encoded in the interrupt translation table entry, so can't be easily derived from just an LPI number (short of walking *all* tables and find the matching LPI). To avoid this in case we need to know the VCPU (for the INVALL command, for instance), put the VCPU ID in the struct pending_irq, so that it is easily accessible. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/include/asm-arm/vgic.h | 3 +++ 1 file changed, 3 insertions(+)