hw/i386/pc_piix: Make piix_intx_routing_notifier_xen() more device independent

[PATCH] hw/i386/pc_piix: Make piix_intx_routing_notifier_xen() more device independent

Posted by Bernhard Beschow 10 months, 3 weeks ago

This is a follow-up on commit 89965db43cce "hw/isa/piix3: Avoid Xen-specific
variant of piix3_write_config()" which introduced
piix_intx_routing_notifier_xen(). This function is implemented in board code but
accesses the PCI configuration space of the PIIX ISA function to determine the
PCI interrupt routes. Avoid this by reusing pci_device_route_intx_to_irq() which
makes piix_intx_routing_notifier_xen() more device-agnostic.

One remaining improvement would be making piix_intx_routing_notifier_xen()
agnostic towards the number of PCI interrupt routes and move it to xen-hvm.
This might be useful for possible Q35 Xen efforts but remains a future exercise
for now.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 042c13cdbc..abfcfe4d2b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -92,13 +92,10 @@ static void piix_intx_routing_notifier_xen(PCIDevice *dev)
 {
     int i;
 
-    /* Scan for updates to PCI link routes (0x60-0x63). */
+    /* Scan for updates to PCI link routes. */
     for (i = 0; i < PIIX_NUM_PIRQS; i++) {
-        uint8_t v = dev->config_read(dev, PIIX_PIRQCA + i, 1);
-        if (v & 0x80) {
-            v = 0;
-        }
-        v &= 0xf;
+        const PCIINTxRoute route = pci_device_route_intx_to_irq(dev, i);
+        const uint8_t v = route.mode == PCI_INTX_ENABLED ? route.irq : 0;
         xen_set_pci_link_route(i, v);
     }
 }
-- 
2.43.0

Re: [PATCH] hw/i386/pc_piix: Make piix_intx_routing_notifier_xen() more device independent

Posted by Bernhard Beschow 10 months, 2 weeks ago


Am 7. Januar 2024 23:16:23 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>This is a follow-up on commit 89965db43cce "hw/isa/piix3: Avoid Xen-specific
>variant of piix3_write_config()" which introduced
>piix_intx_routing_notifier_xen(). This function is implemented in board code but
>accesses the PCI configuration space of the PIIX ISA function to determine the
>PCI interrupt routes. Avoid this by reusing pci_device_route_intx_to_irq() which
>makes piix_intx_routing_notifier_xen() more device-agnostic.
>
>One remaining improvement would be making piix_intx_routing_notifier_xen()
>agnostic towards the number of PCI interrupt routes and move it to xen-hvm.
>This might be useful for possible Q35 Xen efforts but remains a future exercise
>for now.
>
>Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Hi Michael,

could you tag this, too? Or do we need another R-b?

Best regards,
Bernhard

>---
> hw/i386/pc_piix.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>index 042c13cdbc..abfcfe4d2b 100644
>--- a/hw/i386/pc_piix.c
>+++ b/hw/i386/pc_piix.c
>@@ -92,13 +92,10 @@ static void piix_intx_routing_notifier_xen(PCIDevice *dev)
> {
>     int i;
> 
>-    /* Scan for updates to PCI link routes (0x60-0x63). */
>+    /* Scan for updates to PCI link routes. */
>     for (i = 0; i < PIIX_NUM_PIRQS; i++) {
>-        uint8_t v = dev->config_read(dev, PIIX_PIRQCA + i, 1);
>-        if (v & 0x80) {
>-            v = 0;
>-        }
>-        v &= 0xf;
>+        const PCIINTxRoute route = pci_device_route_intx_to_irq(dev, i);
>+        const uint8_t v = route.mode == PCI_INTX_ENABLED ? route.irq : 0;
>         xen_set_pci_link_route(i, v);
>     }
> }

Re: [PATCH] hw/i386/pc_piix: Make piix_intx_routing_notifier_xen() more device independent

Posted by Michael S. Tsirkin 10 months, 2 weeks ago

On Sun, Jan 14, 2024 at 12:21:59PM +0000, Bernhard Beschow wrote:
> 
> 
> Am 7. Januar 2024 23:16:23 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >This is a follow-up on commit 89965db43cce "hw/isa/piix3: Avoid Xen-specific
> >variant of piix3_write_config()" which introduced
> >piix_intx_routing_notifier_xen(). This function is implemented in board code but
> >accesses the PCI configuration space of the PIIX ISA function to determine the
> >PCI interrupt routes. Avoid this by reusing pci_device_route_intx_to_irq() which
> >makes piix_intx_routing_notifier_xen() more device-agnostic.
> >
> >One remaining improvement would be making piix_intx_routing_notifier_xen()
> >agnostic towards the number of PCI interrupt routes and move it to xen-hvm.
> >This might be useful for possible Q35 Xen efforts but remains a future exercise
> >for now.
> >
> >Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> 
> Hi Michael,
> 
> could you tag this, too? Or do we need another R-b?
> 
> Best regards,
> Bernhard

tagged, too.

> >---
> > hw/i386/pc_piix.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> >diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >index 042c13cdbc..abfcfe4d2b 100644
> >--- a/hw/i386/pc_piix.c
> >+++ b/hw/i386/pc_piix.c
> >@@ -92,13 +92,10 @@ static void piix_intx_routing_notifier_xen(PCIDevice *dev)
> > {
> >     int i;
> > 
> >-    /* Scan for updates to PCI link routes (0x60-0x63). */
> >+    /* Scan for updates to PCI link routes. */
> >     for (i = 0; i < PIIX_NUM_PIRQS; i++) {
> >-        uint8_t v = dev->config_read(dev, PIIX_PIRQCA + i, 1);
> >-        if (v & 0x80) {
> >-            v = 0;
> >-        }
> >-        v &= 0xf;
> >+        const PCIINTxRoute route = pci_device_route_intx_to_irq(dev, i);
> >+        const uint8_t v = route.mode == PCI_INTX_ENABLED ? route.irq : 0;
> >         xen_set_pci_link_route(i, v);
> >     }
> > }

Re: [PATCH] hw/i386/pc_piix: Make piix_intx_routing_notifier_xen() more device independent

Posted by Philippe Mathieu-Daudé 10 months, 1 week ago

On 14/1/24 13:25, Michael S. Tsirkin wrote:
> On Sun, Jan 14, 2024 at 12:21:59PM +0000, Bernhard Beschow wrote:
>>
>>
>> Am 7. Januar 2024 23:16:23 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>> This is a follow-up on commit 89965db43cce "hw/isa/piix3: Avoid Xen-specific
>>> variant of piix3_write_config()" which introduced
>>> piix_intx_routing_notifier_xen(). This function is implemented in board code but
>>> accesses the PCI configuration space of the PIIX ISA function to determine the
>>> PCI interrupt routes. Avoid this by reusing pci_device_route_intx_to_irq() which
>>> makes piix_intx_routing_notifier_xen() more device-agnostic.
>>>
>>> One remaining improvement would be making piix_intx_routing_notifier_xen()
>>> agnostic towards the number of PCI interrupt routes and move it to xen-hvm.
>>> This might be useful for possible Q35 Xen efforts but remains a future exercise
>>> for now.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>
>> Hi Michael,
>>
>> could you tag this, too? Or do we need another R-b?
>>
>> Best regards,
>> Bernhard
> 
> tagged, too.

FYI merged as commit ebd92d6de3.

Re: [PATCH] hw/i386/pc_piix: Make piix_intx_routing_notifier_xen() more device independent

Posted by David Woodhouse 10 months, 3 weeks ago

On Mon, 2024-01-08 at 00:16 +0100, Bernhard Beschow wrote:
> This is a follow-up on commit 89965db43cce "hw/isa/piix3: Avoid Xen-specific
> variant of piix3_write_config()" which introduced
> piix_intx_routing_notifier_xen(). This function is implemented in board code but
> accesses the PCI configuration space of the PIIX ISA function to determine the
> PCI interrupt routes. Avoid this by reusing pci_device_route_intx_to_irq() which
> makes piix_intx_routing_notifier_xen() more device-agnostic.
> 
> One remaining improvement would be making piix_intx_routing_notifier_xen()
> agnostic towards the number of PCI interrupt routes and move it to xen-hvm.
> This might be useful for possible Q35 Xen efforts but remains a future exercise
> for now.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

I'm still moderately unhappy that all this code is written with the
apparent assumption that there is only *one* IRQ# which is the target
for a given INTx, when in fact it gets routed to that pin# on the
legacy PIC and a potentially *different* pin# on the I/O APIC.

But you aren't making that worse, so

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>

Re: [PATCH] hw/i386/pc_piix: Make piix_intx_routing_notifier_xen() more device independent

Posted by Bernhard Beschow 10 months, 3 weeks ago


Am 9. Januar 2024 08:51:37 UTC schrieb David Woodhouse <dwmw2@infradead.org>:
>On Mon, 2024-01-08 at 00:16 +0100, Bernhard Beschow wrote:
>> This is a follow-up on commit 89965db43cce "hw/isa/piix3: Avoid Xen-specific
>> variant of piix3_write_config()" which introduced
>> piix_intx_routing_notifier_xen(). This function is implemented in board code but
>> accesses the PCI configuration space of the PIIX ISA function to determine the
>> PCI interrupt routes. Avoid this by reusing pci_device_route_intx_to_irq() which
>> makes piix_intx_routing_notifier_xen() more device-agnostic.
>> 
>> One remaining improvement would be making piix_intx_routing_notifier_xen()
>> agnostic towards the number of PCI interrupt routes and move it to xen-hvm.
>> This might be useful for possible Q35 Xen efforts but remains a future exercise
>> for now.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>
>I'm still moderately unhappy that all this code is written with the
>apparent assumption that there is only *one* IRQ# which is the target
>for a given INTx, when in fact it gets routed to that pin# on the
>legacy PIC and a potentially *different* pin# on the I/O APIC.

Would TYPE_SPLIT_IRQ help in any way?

>
>But you aren't making that worse, so
>
>Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>

Thanks!

Re: [PATCH] hw/i386/pc_piix: Make piix_intx_routing_notifier_xen() more device independent

Posted by Philippe Mathieu-Daudé 10 months, 3 weeks ago

On 8/1/24 00:16, Bernhard Beschow wrote:
> This is a follow-up on commit 89965db43cce "hw/isa/piix3: Avoid Xen-specific
> variant of piix3_write_config()" which introduced
> piix_intx_routing_notifier_xen(). This function is implemented in board code but
> accesses the PCI configuration space of the PIIX ISA function to determine the
> PCI interrupt routes. Avoid this by reusing pci_device_route_intx_to_irq() which
> makes piix_intx_routing_notifier_xen() more device-agnostic.
> 
> One remaining improvement would be making piix_intx_routing_notifier_xen()
> agnostic towards the number of PCI interrupt routes and move it to xen-hvm.
> This might be useful for possible Q35 Xen efforts but remains a future exercise
> for now.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/pc_piix.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 042c13cdbc..abfcfe4d2b 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -92,13 +92,10 @@ static void piix_intx_routing_notifier_xen(PCIDevice *dev)
>   {
>       int i;
>   
> -    /* Scan for updates to PCI link routes (0x60-0x63). */
> +    /* Scan for updates to PCI link routes. */
>       for (i = 0; i < PIIX_NUM_PIRQS; i++) {
> -        uint8_t v = dev->config_read(dev, PIIX_PIRQCA + i, 1);
> -        if (v & 0x80) {
> -            v = 0;
> -        }
> -        v &= 0xf;
> +        const PCIINTxRoute route = pci_device_route_intx_to_irq(dev, i);

This indeed dispatch to piix_route_intx_pin_to_irq().

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +        const uint8_t v = route.mode == PCI_INTX_ENABLED ? route.irq : 0;
>           xen_set_pci_link_route(i, v);
>       }
>   }