Patchwork [v2,4/6] prep: move int-ack register from PReP to Raven PCI emulation

login
register
mail settings
Submitter Hervé Poussineau
Date April 14, 2012, 8:48 p.m.
Message ID <1334436519-3393-5-git-send-email-hpoussin@reactos.org>
Download mbox | patch
Permalink /patch/152553/
State New
Headers show

Comments

Hervé Poussineau - April 14, 2012, 8:48 p.m.
Register is one byte-wide (as per specification), so there is no need to specify endianness.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/ppc_prep.c |   36 ------------------------------------
 hw/prep_pci.c |   14 ++++++++++++++
 2 files changed, 14 insertions(+), 36 deletions(-)
Andreas Färber - April 28, 2012, 6:46 p.m.
Am 14.04.2012 22:48, schrieb Hervé Poussineau:
> Register is one byte-wide (as per specification), so there is no need to specify endianness.

The region was 4 bytes before, now it's 1. What happens when a 4-byte
read is attempted at that address? Do we need to specify the valid
widths for the MemoryRegion? Or is such a read constructed from this
region and (assuming) the return value of an unassigned read?

> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 8b29da9..43847f5 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -25,10 +25,12 @@
>  #include "hw.h"
>  #include "pci.h"
>  #include "pci_host.h"
> +#include "pc.h"

Is that for pic_read_irq()?

Andreas

>  #include "exec-memory.h"
>  
>  typedef struct PRePPCIState {
>      PCIHostState host_state;
> +    MemoryRegion intack;
>      qemu_irq irq[4];
>  } PREPPCIState;
>  
> @@ -67,6 +69,16 @@ static const MemoryRegionOps PPC_PCIIO_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static uint64_t ppc_intack_read(void *opaque, target_phys_addr_t addr,
> +                                unsigned int size)
> +{
> +    return pic_read_irq(isa_pic);
> +}
> +
> +static const MemoryRegionOps PPC_intack_ops = {
> +    .read = ppc_intack_read,
> +};
> +
>  static int prep_map_irq(PCIDevice *pci_dev, int irq_num)
>  {
>      return (irq_num + (pci_dev->devfn >> 3)) & 1;
> @@ -110,6 +122,8 @@ static int raven_pcihost_init(SysBusDevice *dev)
>      memory_region_init_io(&h->mmcfg, &PPC_PCIIO_ops, s, "pciio", 0x00400000);
>      memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);
>  
> +    memory_region_init_io(&s->intack, &PPC_intack_ops, s, "pci-intack", 1);
> +    memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->intack);
>      pci_create_simple(bus, 0, "raven");
>  
>      return 0;
Hervé Poussineau - April 28, 2012, 8:29 p.m.
Andreas Färber a écrit :
> Am 14.04.2012 22:48, schrieb Hervé Poussineau:
>> Register is one byte-wide (as per specification), so there is no need to specify endianness.
> 
> The region was 4 bytes before, now it's 1. What happens when a 4-byte
> read is attempted at that address? Do we need to specify the valid
> widths for the MemoryRegion? Or is such a read constructed from this
> region and (assuming) the return value of an unassigned read?

At first, Linux and IBM 40p firmware only attempt a 1-byte read to this 
address.
In the case a 4-byte read is attempted to address 0xbffffff0, memory 
region layer will still call ppc_intack_read() function with size=4.

However, by changing region size from 4 to 1, you prevent direct reads
of 0xbffffff1..0xbffffff3, which I think is not a big loss (and is 
closer to specification).

> 
>> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
>> index 8b29da9..43847f5 100644
>> --- a/hw/prep_pci.c
>> +++ b/hw/prep_pci.c
>> @@ -25,10 +25,12 @@
>>  #include "hw.h"
>>  #include "pci.h"
>>  #include "pci_host.h"
>> +#include "pc.h"
> 
> Is that for pic_read_irq()?

Yes. All i8259-related functions are declared in pc.h

Hervé
Avi Kivity - April 29, 2012, 8:32 a.m.
On 04/28/2012 09:46 PM, Andreas Färber wrote:
> Am 14.04.2012 22:48, schrieb Hervé Poussineau:
> > Register is one byte-wide (as per specification), so there is no need to specify endianness.
>
> The region was 4 bytes before, now it's 1. What happens when a 4-byte
> read is attempted at that address? Do we need to specify the valid
> widths for the MemoryRegion? Or is such a read constructed from this
> region and (assuming) the return value of an unassigned read?

This area of what happens during access that falls across region
boundaries is very underspecified in qemu; nor is it clear what happens
in real hardware (in all its variations).
Andreas Färber - April 29, 2012, 8:12 p.m.
Am 29.04.2012 10:32, schrieb Avi Kivity:
> On 04/28/2012 09:46 PM, Andreas Färber wrote:
>> Am 14.04.2012 22:48, schrieb Hervé Poussineau:
>>> Register is one byte-wide (as per specification), so there is no need to specify endianness.
>>
>> The region was 4 bytes before, now it's 1. What happens when a 4-byte
>> read is attempted at that address? Do we need to specify the valid
>> widths for the MemoryRegion? Or is such a read constructed from this
>> region and (assuming) the return value of an unassigned read?
> 
> This area of what happens during access that falls across region
> boundaries is very underspecified in qemu; nor is it clear what happens
> in real hardware (in all its variations).

So, what's your conclusion here? Should we add .valid.max_access_size =
1? Or shall I apply as is?

Andreas
Avi Kivity - April 30, 2012, 8:43 a.m.
On 04/29/2012 11:12 PM, Andreas Färber wrote:
> Am 29.04.2012 10:32, schrieb Avi Kivity:
> > On 04/28/2012 09:46 PM, Andreas Färber wrote:
> >> Am 14.04.2012 22:48, schrieb Hervé Poussineau:
> >>> Register is one byte-wide (as per specification), so there is no need to specify endianness.
> >>
> >> The region was 4 bytes before, now it's 1. What happens when a 4-byte
> >> read is attempted at that address? Do we need to specify the valid
> >> widths for the MemoryRegion? Or is such a read constructed from this
> >> region and (assuming) the return value of an unassigned read?
> > 
> > This area of what happens during access that falls across region
> > boundaries is very underspecified in qemu; nor is it clear what happens
> > in real hardware (in all its variations).
>
> So, what's your conclusion here? Should we add .valid.max_access_size =
> 1? Or shall I apply as is?

If the specification says 1 byte and there are no known guests that
abuse it, I say change it.  Regardless, we need to collect requirements
for this shady area and implement them.
Andreas Färber - April 30, 2012, 5:51 p.m.
Am 30.04.2012 10:43, schrieb Avi Kivity:
> On 04/29/2012 11:12 PM, Andreas Färber wrote:
>> Am 29.04.2012 10:32, schrieb Avi Kivity:
>>> On 04/28/2012 09:46 PM, Andreas Färber wrote:
>>>> Am 14.04.2012 22:48, schrieb Hervé Poussineau:
>>>>> Register is one byte-wide (as per specification), so there is no need to specify endianness.
>>>>
>>>> The region was 4 bytes before, now it's 1. What happens when a 4-byte
>>>> read is attempted at that address? Do we need to specify the valid
>>>> widths for the MemoryRegion? Or is such a read constructed from this
>>>> region and (assuming) the return value of an unassigned read?
>>>
>>> This area of what happens during access that falls across region
>>> boundaries is very underspecified in qemu; nor is it clear what happens
>>> in real hardware (in all its variations).
>>
>> So, what's your conclusion here? Should we add .valid.max_access_size =
>> 1? Or shall I apply as is?
> 
> If the specification says 1 byte and there are no known guests that
> abuse it, I say change it.

Thanks, applied to prep-up branch (with .max_access_size = 1 and
autobreaking the commit message):
http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/prep-up

Andreas

Patch

diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index 30029f9..6a0d81d 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -85,38 +85,6 @@  static int ne2000_irq[NE2000_NB_MAX] = { 9, 10, 11, 3, 4, 5 };
 /* ISA IO ports bridge */
 #define PPC_IO_BASE 0x80000000
 
-/* PCI intack register */
-/* Read-only register (?) */
-static void PPC_intack_write (void *opaque, target_phys_addr_t addr,
-                              uint64_t value, unsigned size)
-{
-#if 0
-    printf("%s: 0x" TARGET_FMT_plx " => 0x%08" PRIx64 "\n", __func__, addr,
-           value);
-#endif
-}
-
-static uint64_t PPC_intack_read(void *opaque, target_phys_addr_t addr,
-                                unsigned size)
-{
-    uint32_t retval = 0;
-
-    if ((addr & 0xf) == 0)
-        retval = pic_read_irq(isa_pic);
-#if 0
-    printf("%s: 0x" TARGET_FMT_plx " <= %08" PRIx32 "\n", __func__, addr,
-           retval);
-#endif
-
-    return retval;
-}
-
-static const MemoryRegionOps PPC_intack_ops = {
-    .read = PPC_intack_read,
-    .write = PPC_intack_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 /* PowerPC control and status registers */
 #if 0 // Not used
 static struct {
@@ -472,7 +440,6 @@  static void ppc_prep_init (ram_addr_t ram_size,
     nvram_t nvram;
     M48t59State *m48t59;
     MemoryRegion *PPC_io_memory = g_new(MemoryRegion, 1);
-    MemoryRegion *intack = g_new(MemoryRegion, 1);
 #if 0
     MemoryRegion *xcsr = g_new(MemoryRegion, 1);
 #endif
@@ -658,9 +625,6 @@  static void ppc_prep_init (ram_addr_t ram_size,
     register_ioport_write(0x0092, 0x01, 1, &PREP_io_800_writeb, sysctrl);
     register_ioport_read(0x0800, 0x52, 1, &PREP_io_800_readb, sysctrl);
     register_ioport_write(0x0800, 0x52, 1, &PREP_io_800_writeb, sysctrl);
-    /* PCI intack location */
-    memory_region_init_io(intack, &PPC_intack_ops, NULL, "ppc-intack", 4);
-    memory_region_add_subregion(sysmem, 0xBFFFFFF0, intack);
     /* PowerPC control and status register group */
 #if 0
     memory_region_init_io(xcsr, &PPC_XCSR_ops, NULL, "ppc-xcsr", 0x1000);
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 8b29da9..43847f5 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -25,10 +25,12 @@ 
 #include "hw.h"
 #include "pci.h"
 #include "pci_host.h"
+#include "pc.h"
 #include "exec-memory.h"
 
 typedef struct PRePPCIState {
     PCIHostState host_state;
+    MemoryRegion intack;
     qemu_irq irq[4];
 } PREPPCIState;
 
@@ -67,6 +69,16 @@  static const MemoryRegionOps PPC_PCIIO_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static uint64_t ppc_intack_read(void *opaque, target_phys_addr_t addr,
+                                unsigned int size)
+{
+    return pic_read_irq(isa_pic);
+}
+
+static const MemoryRegionOps PPC_intack_ops = {
+    .read = ppc_intack_read,
+};
+
 static int prep_map_irq(PCIDevice *pci_dev, int irq_num)
 {
     return (irq_num + (pci_dev->devfn >> 3)) & 1;
@@ -110,6 +122,8 @@  static int raven_pcihost_init(SysBusDevice *dev)
     memory_region_init_io(&h->mmcfg, &PPC_PCIIO_ops, s, "pciio", 0x00400000);
     memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);
 
+    memory_region_init_io(&s->intack, &PPC_intack_ops, s, "pci-intack", 1);
+    memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->intack);
     pci_create_simple(bus, 0, "raven");
 
     return 0;