[PATCH 0/6] hw/pci-host/sabre: Report UNIMP/GUEST_ERROR accesses

[PATCH 0/6] hw/pci-host/sabre: Report UNIMP/GUEST_ERROR accesses

[Philippe Mathieu-Daudé]

Notes while trying to understand Mark's patch from yesterday:
"sabre: increase number of PCI bus IRQs from 32 to 64"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg749458.html

Philippe Mathieu-Daudé (6):
  hw/pci-host/sabre: Update documentation link
  hw/pci-host/sabre: Remove superfluous address range check
  hw/pci-host/sabre: Simplify code initializing variable once
  hw/pci-host/sabre: Report unimplemented accesses via UNIMP log_mask
  hw/pci-host/sabre: Report IOMMU address range as unimplemented
  hw/pci-host/sabre: Log reserved address accesses as GUEST_ERROR

 hw/pci-host/sabre.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

-- 
2.26.2

[PATCH 1/6] hw/pci-host/sabre: Update documentation link

[Philippe Mathieu-Daudé]

The current link redirects to https://www.oracle.com/sun/
announcing "Oracle acquired Sun Microsystems in 2010, ..."
but does not give hint where to find the datasheet.

Use the archived PDF on the Wayback Machine, which works.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/sabre.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 5ac62836238..3634f8369b7 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -44,7 +44,7 @@
 /*
  * Chipset docs:
  * PBM: "UltraSPARC IIi User's Manual",
- * http://www.sun.com/processors/manuals/805-0087.pdf
+ * https://web.archive.org/web/20030403110020/http://www.sun.com/processors/manuals/805-0087.pdf
  */
 
 #define PBM_PCI_IMR_MASK    0x7fffffff
-- 
2.26.2

Re: [PATCH 1/6] hw/pci-host/sabre: Update documentation link

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:

> The current link redirects to https://www.oracle.com/sun/
> announcing "Oracle acquired Sun Microsystems in 2010, ..."
> but does not give hint where to find the datasheet.
> 
> Use the archived PDF on the Wayback Machine, which works.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/pci-host/sabre.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
> index 5ac62836238..3634f8369b7 100644
> --- a/hw/pci-host/sabre.c
> +++ b/hw/pci-host/sabre.c
> @@ -44,7 +44,7 @@
>   /*
>    * Chipset docs:
>    * PBM: "UltraSPARC IIi User's Manual",
> - * http://www.sun.com/processors/manuals/805-0087.pdf
> + * https://web.archive.org/web/20030403110020/http://www.sun.com/processors/manuals/805-0087.pdf
>    */
>   
>   #define PBM_PCI_IMR_MASK    0x7fffffff

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 2/6] hw/pci-host/sabre: Remove superfluous address range check

[Philippe Mathieu-Daudé]

The region is registered as 64KiB in sabre_init():

    memory_region_init_io(&s->sabre_config, OBJECT(s), &sabre_config_ops, s,
                          "sabre-config", 0x10000);

Remove the superfluous check.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/sabre.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 3634f8369b7..0889c9369f6 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -120,7 +120,7 @@ static void sabre_config_write(void *opaque, hwaddr addr,
 
     trace_sabre_config_write(addr, val);
 
-    switch (addr & 0xffff) {
+    switch (addr) {
     case 0x30 ... 0x4f: /* DMA error registers */
         /* XXX: not implemented yet */
         break;
@@ -197,7 +197,7 @@ static uint64_t sabre_config_read(void *opaque,
     SabreState *s = opaque;
     uint32_t val;
 
-    switch (addr & 0xffff) {
+    switch (addr) {
     case 0x30 ... 0x4f: /* DMA error registers */
         val = 0;
         /* XXX: not implemented yet */
-- 
2.26.2

Re: [PATCH 2/6] hw/pci-host/sabre: Remove superfluous address range check

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:

> The region is registered as 64KiB in sabre_init():
> 
>      memory_region_init_io(&s->sabre_config, OBJECT(s), &sabre_config_ops, s,
>                            "sabre-config", 0x10000);
> 
> Remove the superfluous check.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/pci-host/sabre.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
> index 3634f8369b7..0889c9369f6 100644
> --- a/hw/pci-host/sabre.c
> +++ b/hw/pci-host/sabre.c
> @@ -120,7 +120,7 @@ static void sabre_config_write(void *opaque, hwaddr addr,
>   
>       trace_sabre_config_write(addr, val);
>   
> -    switch (addr & 0xffff) {
> +    switch (addr) {
>       case 0x30 ... 0x4f: /* DMA error registers */
>           /* XXX: not implemented yet */
>           break;
> @@ -197,7 +197,7 @@ static uint64_t sabre_config_read(void *opaque,
>       SabreState *s = opaque;
>       uint32_t val;
>   
> -    switch (addr & 0xffff) {
> +    switch (addr) {
>       case 0x30 ... 0x4f: /* DMA error registers */
>           val = 0;
>           /* XXX: not implemented yet */

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 3/6] hw/pci-host/sabre: Simplify code initializing variable once

[Philippe Mathieu-Daudé]

We only need to zero-initialize 'val' once.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/sabre.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 0889c9369f6..3645bc962cb 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -195,32 +195,25 @@ static uint64_t sabre_config_read(void *opaque,
                                   hwaddr addr, unsigned size)
 {
     SabreState *s = opaque;
-    uint32_t val;
+    uint32_t val = 0;
 
     switch (addr) {
     case 0x30 ... 0x4f: /* DMA error registers */
-        val = 0;
         /* XXX: not implemented yet */
         break;
     case 0xc00 ... 0xc3f: /* PCI interrupt control */
         if (addr & 4) {
             val = s->pci_irq_map[(addr & 0x3f) >> 3];
-        } else {
-            val = 0;
         }
         break;
     case 0x1000 ... 0x107f: /* OBIO interrupt control */
         if (addr & 4) {
             val = s->obio_irq_map[(addr & 0xff) >> 3];
-        } else {
-            val = 0;
         }
         break;
     case 0x1080 ... 0x108f: /* PCI bus error */
         if (addr & 4) {
             val = s->pci_err_irq_map[(addr & 0xf) >> 3];
-        } else {
-            val = 0;
         }
         break;
     case 0x2000 ... 0x202f: /* PCI control */
@@ -229,8 +222,6 @@ static uint64_t sabre_config_read(void *opaque,
     case 0xf020 ... 0xf027: /* Reset control */
         if (addr & 4) {
             val = s->reset_control;
-        } else {
-            val = 0;
         }
         break;
     case 0x5000 ... 0x51cf: /* PIO/DMA diagnostics */
@@ -239,7 +230,6 @@ static uint64_t sabre_config_read(void *opaque,
     case 0xf000 ... 0xf01f: /* FFB config, memory control */
         /* we don't care */
     default:
-        val = 0;
         break;
     }
     trace_sabre_config_read(addr, val);
-- 
2.26.2

Re: [PATCH 3/6] hw/pci-host/sabre: Simplify code initializing variable once

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:

> We only need to zero-initialize 'val' once.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/pci-host/sabre.c | 12 +-----------
>   1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
> index 0889c9369f6..3645bc962cb 100644
> --- a/hw/pci-host/sabre.c
> +++ b/hw/pci-host/sabre.c
> @@ -195,32 +195,25 @@ static uint64_t sabre_config_read(void *opaque,
>                                     hwaddr addr, unsigned size)
>   {
>       SabreState *s = opaque;
> -    uint32_t val;
> +    uint32_t val = 0;
>   
>       switch (addr) {
>       case 0x30 ... 0x4f: /* DMA error registers */
> -        val = 0;
>           /* XXX: not implemented yet */
>           break;
>       case 0xc00 ... 0xc3f: /* PCI interrupt control */
>           if (addr & 4) {
>               val = s->pci_irq_map[(addr & 0x3f) >> 3];
> -        } else {
> -            val = 0;
>           }
>           break;
>       case 0x1000 ... 0x107f: /* OBIO interrupt control */
>           if (addr & 4) {
>               val = s->obio_irq_map[(addr & 0xff) >> 3];
> -        } else {
> -            val = 0;
>           }
>           break;
>       case 0x1080 ... 0x108f: /* PCI bus error */
>           if (addr & 4) {
>               val = s->pci_err_irq_map[(addr & 0xf) >> 3];
> -        } else {
> -            val = 0;
>           }
>           break;
>       case 0x2000 ... 0x202f: /* PCI control */
> @@ -229,8 +222,6 @@ static uint64_t sabre_config_read(void *opaque,
>       case 0xf020 ... 0xf027: /* Reset control */
>           if (addr & 4) {
>               val = s->reset_control;
> -        } else {
> -            val = 0;
>           }
>           break;
>       case 0x5000 ... 0x51cf: /* PIO/DMA diagnostics */
> @@ -239,7 +230,6 @@ static uint64_t sabre_config_read(void *opaque,
>       case 0xf000 ... 0xf01f: /* FFB config, memory control */
>           /* we don't care */
>       default:
> -        val = 0;
>           break;
>       }
>       trace_sabre_config_read(addr, val);

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 4/6] hw/pci-host/sabre: Report unimplemented accesses via UNIMP log_mask

[Philippe Mathieu-Daudé]

Report unimplemented register accesses using qemu_log_mask(UNIMP).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/sabre.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 3645bc962cb..4412e23131c 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -121,8 +121,10 @@ static void sabre_config_write(void *opaque, hwaddr addr,
     trace_sabre_config_write(addr, val);
 
     switch (addr) {
-    case 0x30 ... 0x4f: /* DMA error registers */
-        /* XXX: not implemented yet */
+    case  0x30 ...  0x4f: /* DMA error registers */
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
+                      __func__, addr);
         break;
     case 0xc00 ... 0xc3f: /* PCI interrupt control */
         if (addr & 4) {
@@ -198,8 +200,10 @@ static uint64_t sabre_config_read(void *opaque,
     uint32_t val = 0;
 
     switch (addr) {
-    case 0x30 ... 0x4f: /* DMA error registers */
-        /* XXX: not implemented yet */
+    case  0x30 ...  0x4f: /* DMA error registers */
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
+                      __func__, addr);
         break;
     case 0xc00 ... 0xc3f: /* PCI interrupt control */
         if (addr & 4) {
-- 
2.26.2

Re: [PATCH 4/6] hw/pci-host/sabre: Report unimplemented accesses via UNIMP log_mask

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:

> Report unimplemented register accesses using qemu_log_mask(UNIMP).
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/pci-host/sabre.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
> index 3645bc962cb..4412e23131c 100644
> --- a/hw/pci-host/sabre.c
> +++ b/hw/pci-host/sabre.c
> @@ -121,8 +121,10 @@ static void sabre_config_write(void *opaque, hwaddr addr,
>       trace_sabre_config_write(addr, val);
>   
>       switch (addr) {
> -    case 0x30 ... 0x4f: /* DMA error registers */
> -        /* XXX: not implemented yet */
> +    case  0x30 ...  0x4f: /* DMA error registers */
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
> +                      __func__, addr);
>           break;
>       case 0xc00 ... 0xc3f: /* PCI interrupt control */
>           if (addr & 4) {
> @@ -198,8 +200,10 @@ static uint64_t sabre_config_read(void *opaque,
>       uint32_t val = 0;
>   
>       switch (addr) {
> -    case 0x30 ... 0x4f: /* DMA error registers */
> -        /* XXX: not implemented yet */
> +    case  0x30 ...  0x4f: /* DMA error registers */
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
> +                      __func__, addr);
>           break;
>       case 0xc00 ... 0xc3f: /* PCI interrupt control */
>           if (addr & 4) {

It seems as if there are quite a few other registers that haven't been implemented 
here which aren't mentioned in the comments. My preference would be to rework this 
patch so that the comments for the unimplemented registers are all at the end of the 
switch() with the fallthrough to default, and then update patch 6 to use LOG_UNIMP so 
everything is logged in one place.


ATB,

Mark.

[PATCH 5/6] hw/pci-host/sabre: Report IOMMU address range as unimplemented

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/sabre.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 4412e23131c..67699ac9058 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -122,6 +122,7 @@ static void sabre_config_write(void *opaque, hwaddr addr,
 
     switch (addr) {
     case  0x30 ...  0x4f: /* DMA error registers */
+    case 0x200 ... 0x21f: /* IOMMU registers */
         qemu_log_mask(LOG_UNIMP,
                       "%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
                       __func__, addr);
@@ -201,6 +202,7 @@ static uint64_t sabre_config_read(void *opaque,
 
     switch (addr) {
     case  0x30 ...  0x4f: /* DMA error registers */
+    case 0x200 ... 0x21f: /* IOMMU registers */
         qemu_log_mask(LOG_UNIMP,
                       "%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
                       __func__, addr);
-- 
2.26.2

Re: [PATCH 5/6] hw/pci-host/sabre: Report IOMMU address range as unimplemented

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/pci-host/sabre.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
> index 4412e23131c..67699ac9058 100644
> --- a/hw/pci-host/sabre.c
> +++ b/hw/pci-host/sabre.c
> @@ -122,6 +122,7 @@ static void sabre_config_write(void *opaque, hwaddr addr,
>   
>       switch (addr) {
>       case  0x30 ...  0x4f: /* DMA error registers */
> +    case 0x200 ... 0x21f: /* IOMMU registers */
>           qemu_log_mask(LOG_UNIMP,
>                         "%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
>                         __func__, addr);
> @@ -201,6 +202,7 @@ static uint64_t sabre_config_read(void *opaque,
>   
>       switch (addr) {
>       case  0x30 ...  0x4f: /* DMA error registers */
> +    case 0x200 ... 0x21f: /* IOMMU registers */
>           qemu_log_mask(LOG_UNIMP,
>                         "%s: Register 0x%02" HWADDR_PRIX " not implemented\n",
>                         __func__, addr);

In theory this should never happen since a reference to the IOMMU should always be 
set using an object property link (i.e. it is a developer error rather than an 
unimplemented error) and its memory region overlaps this space within the PCI host 
bridge.

Rather than add these logging statemants and/or failing if the property is not set, I 
think now it may be possible to simply embed the IOMMU device within sabre itself 
using the updated QOM APIs. I can take a look to see if this approach will work later 
in the week.


ATB,

Mark.

[PATCH 6/6] hw/pci-host/sabre: Log reserved address accesses as GUEST_ERROR

[Philippe Mathieu-Daudé]

Report accesses to reserved registers using qemu_log_mask(GUEST_ERROR).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/sabre.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
index 67699ac9058..cc97c266a57 100644
--- a/hw/pci-host/sabre.c
+++ b/hw/pci-host/sabre.c
@@ -189,7 +189,11 @@ static void sabre_config_write(void *opaque, hwaddr addr,
     case 0xa800 ... 0xa80f: /* Interrupt diagnostics */
     case 0xf000 ... 0xf01f: /* FFB config, memory control */
         /* we don't care */
+        break;
     default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Register 0x%04" HWADDR_PRIX " is reserved\n",
+                      __func__, addr);
         break;
     }
 }
@@ -235,7 +239,11 @@ static uint64_t sabre_config_read(void *opaque,
     case 0xa800 ... 0xa80f: /* Interrupt diagnostics */
     case 0xf000 ... 0xf01f: /* FFB config, memory control */
         /* we don't care */
+        break;
     default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Register 0x%04" HWADDR_PRIX " is reserved\n",
+                      __func__, addr);
         break;
     }
     trace_sabre_config_read(addr, val);
-- 
2.26.2

Re: [PATCH 6/6] hw/pci-host/sabre: Log reserved address accesses as GUEST_ERROR

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:

> Report accesses to reserved registers using qemu_log_mask(GUEST_ERROR).
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/pci-host/sabre.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
> index 67699ac9058..cc97c266a57 100644
> --- a/hw/pci-host/sabre.c
> +++ b/hw/pci-host/sabre.c
> @@ -189,7 +189,11 @@ static void sabre_config_write(void *opaque, hwaddr addr,
>       case 0xa800 ... 0xa80f: /* Interrupt diagnostics */
>       case 0xf000 ... 0xf01f: /* FFB config, memory control */
>           /* we don't care */
> +        break;
>       default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Register 0x%04" HWADDR_PRIX " is reserved\n",
> +                      __func__, addr);
>           break;
>       }
>   }
> @@ -235,7 +239,11 @@ static uint64_t sabre_config_read(void *opaque,
>       case 0xa800 ... 0xa80f: /* Interrupt diagnostics */
>       case 0xf000 ... 0xf01f: /* FFB config, memory control */
>           /* we don't care */
> +        break;
>       default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Register 0x%04" HWADDR_PRIX " is reserved\n",
> +                      __func__, addr);
>           break;
>       }
>       trace_sabre_config_read(addr, val);

As per my comment on patch 4, I think these should be logged at LOG_UNIMP and the 
message changed to "is unimplemented". Other than that:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH 0/6] hw/pci-host/sabre: Report UNIMP/GUEST_ERROR accesses

[Mark Cave-Ayland]

On 12/10/2020 18:09, Philippe Mathieu-Daudé wrote:

> Notes while trying to understand Mark's patch from yesterday:
> "sabre: increase number of PCI bus IRQs from 32 to 64"
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg749458.html
> 
> Philippe Mathieu-Daudé (6):
>    hw/pci-host/sabre: Update documentation link
>    hw/pci-host/sabre: Remove superfluous address range check
>    hw/pci-host/sabre: Simplify code initializing variable once
>    hw/pci-host/sabre: Report unimplemented accesses via UNIMP log_mask
>    hw/pci-host/sabre: Report IOMMU address range as unimplemented
>    hw/pci-host/sabre: Log reserved address accesses as GUEST_ERROR
> 
>   hw/pci-host/sabre.c | 40 ++++++++++++++++++++++------------------
>   1 file changed, 22 insertions(+), 18 deletions(-)

Thanks for this - I've applied patches 1-3 to my qemu-sparc branch with some comments 
on patches 4-6. I should be able to look at these later in the week if you're 
currently busy.


ATB,

Mark.