| Message ID | 1334436519-3393-5-git-send-email-hpoussin@reactos.org |
|---|---|
| State | New |
| Headers | show |
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;
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é
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).
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
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.
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
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;
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(-)