Refactor portio dispatching
cover Refactor portio dispatching
This series converts the remaining users of register_ioport* to portio lists, simplifies the handling of subpages and adds support for unaligned memory region accesses. Then it replaces the current portio dispatcher with the existing one for MMIO and removes several lines of code. This also allows to build BQL-free portio on top once we enhance the memory layer accordingly.
Seems to work fine so far but surely requires thorough review. And I would welcome early comments on the direction.
Jan
Jan Kiszka (15):
adlib: replace register_ioport*
applesmc: replace register_ioport*
wdt_ib700: replace register_ioport*
i82374: replace register_ioport*
prep: replace register_ioport*
vt82c686: replace register_ioport*
Privatize register_ioport_read/write
isa: implement isa_is_ioport_assigned via memory_region_find
memory: Introduce address_space_lookup_region
memory: Rework sub-page handling
memory: Allow unaligned address_space_rw
vmware-vga: Accept unaligned I/O accesses
ioport: Switch dispatching to memory core layer
ioport: Remove unused old dispatching services
ioport: Move IOPortRead/WriteFunc typedefs to memory.h
cputlb.c | 2 +- exec.c | 273 +++++++++++++------------------------ hw/acpi/piix4.c | 6 +- hw/audio/adlib.c | 20 ++- hw/display/vmware_vga.c | 4 + hw/dma/i82374.c | 17 ++- hw/isa/isa-bus.c | 11 ++ hw/isa/lpc_ich9.c | 8 +- hw/isa/vt82c686.c | 40 ++++-- hw/misc/applesmc.c | 48 +++++-- hw/ppc/prep.c | 23 ++- hw/watchdog/wdt_ib700.c | 12 ++- include/exec/cputlb.h | 2 - include/exec/ioport.h | 19 +--- include/exec/iorange.h | 31 ---- include/exec/memory-internal.h | 2 - include/exec/memory.h | 26 ++-- include/hw/isa/isa.h | 2 + ioport.c | 294 ++++------------------------------------ memory.c | 102 +++++--------- translate-all.c | 3 +- 21 files changed, 311 insertions(+), 634 deletions(-) delete mode 100644 include/exec/iorange.h
Signature · 1 line
1.7.3.4
RFC PATCH 1/15 adlib: replace register_ioport*
Convert over to memory regions to obsolete register_ioport*.
--- hw/audio/adlib.c | 20 ++++++++++++-------- 1 files changed, 12 insertions(+), 8 deletions(-)
hw/audio/adlib.c +12 -8
@@ -283,9 +283,17 @@ static void Adlib_fini (AdlibState *s)AUD_remove_card (&s->card); } +static MemoryRegionPortio adlib_portio_list[] = { + {0x388 ,4 ,1 , .read = adlib_read, .write = adlib_write, }, + {0 ,4 ,1 , .read = adlib_read, .write = adlib_write, }, + {0 ,2 ,1 , .read = adlib_read, .write = adlib_write, }, +PORTIO_END_OF_LIST (), +}; +static int Adlib_initfn (ISADevice *dev) {AdlibState *s =ADLIB (dev); +PortioList *port_list =g_new (PortioList,1 );struct audsettings as;if (glob_adlib) {
@@ -338,14 +346,10 @@ static int Adlib_initfn (ISADevice *dev) s->samples =AUD_get_buffer_size_out (s->voice) >>SHIFT ; s->mixbuf =g_malloc0 (s->samples <<SHIFT ); -register_ioport_read (0x388 ,4 ,1 , adlib_read, s); -register_ioport_write (0x388 ,4 ,1 , adlib_write, s); - -register_ioport_read (s->port,4 ,1 , adlib_read, s); -register_ioport_write (s->port,4 ,1 , adlib_write, s); - -register_ioport_read (s->port +8 ,2 ,1 , adlib_read, s); -register_ioport_write (s->port +8 ,2 ,1 , adlib_write, s); + adlib_portio_list[1 ].offset = s->port; + adlib_portio_list[2 ].offset = s->port +8 ; +portio_list_init (port_list, adlib_portio_list, s,"adlib" ); +portio_list_add (port_list,isa_address_space_io (dev),0 );return 0 ; }
Signature · 1 line
1.7.3.4
RFC PATCH 2/15 applesmc: replace register_ioport*
In reply to Jan KiszkaConvert over to memory regions to obsolete register_ioport*.
--- hw/misc/applesmc.c | 48 ++++++++++++++++++++++++++++++++++++------------ 1 files changed, 36 insertions(+), 12 deletions(-)
hw/misc/applesmc.c +36 -12
@@ -73,6 +73,8 @@ typedef struct AppleSMCState AppleSMCState;struct AppleSMCState {ISADevice parent_obj; +MemoryRegion io_data; +MemoryRegion io_cmd;uint32_t iobase;uint8_t cmd;uint8_t status;
@@ -86,7 +88,8 @@ struct AppleSMCState { QLIST_HEAD(, AppleSMCData) data_def; }; -static void applesmc_io_cmd_writeb (void *opaque,uint32_t addr,uint32_t val) +static void applesmc_io_cmd_write (void *opaque,hwaddr addr,uint64_t val, +unsigned size) {AppleSMCState *s = opaque;
@@ -115,7 +118,8 @@ static void applesmc_fill_data(AppleSMCState *s) } } -static void applesmc_io_data_writeb (void *opaque,uint32_t addr,uint32_t val) +static void applesmc_io_data_write (void *opaque,hwaddr addr,uint64_t val, +unsigned size) {AppleSMCState *s = opaque;
@@ -138,7 +142,8 @@ static void applesmc_io_data_writeb(void *opaque, uint32_t addr, uint32_t val) } } -static uint32_t applesmc_io_data_readb (void *opaque,uint32_t addr1) +static uint64_t applesmc_io_data_read (void *opaque,hwaddr addr1, +unsigned size) {AppleSMCState *s = opaque;uint8_t retval =0 ;
@@ -162,7 +167,7 @@ static uint32_t applesmc_io_data_readb(void *opaque, uint32_t addr1)return retval; } -static uint32_t applesmc_io_cmd_readb (void *opaque,uint32_t addr1) +static uint64_t applesmc_io_cmd_read (void *opaque,hwaddr addr1,unsigned size) {AppleSMCState *s = opaque;
@@ -201,18 +206,37 @@ static void qdev_applesmc_isa_reset(DeviceState *dev)applesmc_add_key (s,"MSSD" ,1 ,"\0x3" ); } +static const MemoryRegionOps applesmc_data_io_ops = { + .write = applesmc_io_data_write, + .read = applesmc_io_data_read, + .endianness =DEVICE_NATIVE_ENDIAN , + .impl = { + .min_access_size =1 , + .max_access_size =1 , + }, +}; + +static const MemoryRegionOps applesmc_cmd_io_ops = { + .write = applesmc_io_cmd_write, + .read = applesmc_io_cmd_read, + .endianness =DEVICE_NATIVE_ENDIAN , + .impl = { + .min_access_size =1 , + .max_access_size =1 , + }, +}; +static int applesmc_isa_init (ISADevice *dev) {AppleSMCState *s =APPLE_SMC (dev); -register_ioport_read (s->iobase +APPLESMC_DATA_PORT ,4 ,1 , - applesmc_io_data_readb, s); -register_ioport_read (s->iobase +APPLESMC_CMD_PORT ,4 ,1 , - applesmc_io_cmd_readb, s); -register_ioport_write (s->iobase +APPLESMC_DATA_PORT ,4 ,1 , - applesmc_io_data_writeb, s); -register_ioport_write (s->iobase +APPLESMC_CMD_PORT ,4 ,1 , - applesmc_io_cmd_writeb, s); +memory_region_init_io (&s->io_data, &applesmc_data_io_ops, s, +"applesmc-data" ,4 ); +isa_register_ioport (dev, &s->io_data, s->iobase +APPLESMC_DATA_PORT ); + +memory_region_init_io (&s->io_cmd, &applesmc_cmd_io_ops, s, +"applesmc-cmd" ,4 ); +isa_register_ioport (dev, &s->io_cmd, s->iobase +APPLESMC_CMD_PORT );if (!s->osk || (strlen (s->osk) !=64 )) {fprintf (stderr,"WARNING: Using AppleSMC with invalid key\n" );
Signature · 1 line
1.7.3.4
RFC PATCH 3/15 wdt_ib700: replace register_ioport*
In reply to Jan KiszkaConvert over to memory regions to obsolete register_ioport*.
--- hw/watchdog/wdt_ib700.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-)
hw/watchdog/wdt_ib700.c +10 -2
@@ -97,15 +97,23 @@ static const VMStateDescription vmstate_ib700 = { } }; +static const MemoryRegionPortio wdt_portio_list[] = { + {0x441 ,2 ,1 , .write = ib700_write_disable_reg, }, + {0x443 ,2 ,1 , .write = ib700_write_enable_reg, }, +PORTIO_END_OF_LIST (), +}; +static int wdt_ib700_init (ISADevice *dev) {IB700State *s =IB700 (dev); +PortioList *port_list =g_new (PortioList,1 );ib700_debug ("watchdog init\n" ); s->timer =qemu_new_timer_ns (vm_clock, ib700_timer_expired, s); -register_ioport_write (0x441 ,2 ,1 , ib700_write_disable_reg, s); -register_ioport_write (0x443 ,2 ,1 , ib700_write_enable_reg, s); + +portio_list_init (port_list, wdt_portio_list, s,"ib700" ); +portio_list_add (port_list,isa_address_space_io (dev),0 );return 0 ; }
Signature · 1 line
1.7.3.4
RFC PATCH 4/15 i82374: replace register_ioport*
In reply to Jan KiszkaConvert over to memory regions to obsolete register_ioport*.
--- hw/dma/i82374.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-)
hw/dma/i82374.c +12 -5
@@ -124,16 +124,23 @@ static const VMStateDescription vmstate_isa_i82374 = { }, }; +static const MemoryRegionPortio i82374_portio_list[] = { + {0x0A ,1 ,1 , .read = i82374_read_isr, }, + {0x10 ,8 ,1 , .write = i82374_write_command, }, + {0x18 ,8 ,1 , .read = i82374_read_status, }, + {0x20 ,0x20 ,1 , + .write = i82374_write_descriptor, .read = i82374_read_descriptor, }, +PORTIO_END_OF_LIST (), +}; +static int i82374_isa_init (ISADevice *dev) {ISAi82374State *isa =I82374 (dev);I82374State *s = &isa->state; +PortioList *port_list =g_new (PortioList,1 ); -register_ioport_read (isa->iobase +0x0A ,1 ,1 , i82374_read_isr, s); -register_ioport_write (isa->iobase +0x10 ,8 ,1 , i82374_write_command, s); -register_ioport_read (isa->iobase +0x18 ,8 ,1 , i82374_read_status, s); -register_ioport_write (isa->iobase +0x20 ,0x20 ,1 , i82374_write_descriptor, s); -register_ioport_read (isa->iobase +0x20 ,0x20 ,1 , i82374_read_descriptor, s); +portio_list_init (port_list, i82374_portio_list, s,"i82374" ); +portio_list_add (port_list,isa_address_space_io (dev), isa->iobase);i82374_init (s);
Signature · 1 line
1.7.3.4
RFC PATCH 5/15 prep: replace register_ioport*
In reply to Jan KiszkaConvert over to memory regions to obsolete register_ioport*.
--- hw/ppc/prep.c | 23 +++++++++++++++-------- 1 files changed, 15 insertions(+), 8 deletions(-)
hw/ppc/prep.c +15 -8
@@ -429,6 +429,16 @@ static void ppc_prep_reset(void *opaque)cpu_reset (CPU (cpu)); } +static const MemoryRegionPortio prep_portio_list[] = { +/* System control ports */ + {0x0092 ,1 ,1 , .read = PREP_io_800_readb, .write = PREP_io_800_writeb, }, + {0x0800 ,0x52 ,1 , + .read = PREP_io_800_readb, .write = PREP_io_800_writeb, }, +/* Special port to get debug messages from Open-Firmware */ + {0x0F00 ,4 ,1 , .write = PPC_debug_write, }, +PORTIO_END_OF_LIST (), +}; +/* PowerPC PREP hardware initialisation */ static void ppc_prep_init (QEMUMachineInitArgs *args) {
@@ -445,6 +455,7 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)nvram_t nvram;M48t59State *m48t59;MemoryRegion *PPC_io_memory =g_new (MemoryRegion,1 ); +PortioList *port_list =g_new (PortioList,1 );#if 0 MemoryRegion *xcsr =g_new (MemoryRegion,1 );#endif
@@ -624,11 +635,10 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)isa_create_simple (isa_bus,"i8042" ); sysctrl->reset_irq = first_cpu->irq_inputs[PPC6xx_INPUT_HRESET]; -/* System control ports */ -register_ioport_read (0x0092 ,0x01 ,1 , &PREP_io_800_readb, sysctrl); -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); + +portio_list_init (port_list, prep_portio_list, sysctrl,"prep" ); +portio_list_add (port_list,get_system_io (),0x0 ); +/* PowerPC control and status register group */ #if 0 memory_region_init_io (xcsr, &PPC_XCSR_ops,NULL ,"ppc-xcsr" ,0x1000 );
@@ -655,9 +665,6 @@ static void ppc_prep_init(QEMUMachineInitArgs *args)/* XXX: need an option to load a NVRAM image */ 0 , graphic_width, graphic_height, graphic_depth); - -/* Special port to get debug messages from Open-Firmware */ -register_ioport_write (0x0F00 ,4 ,1 , &PPC_debug_write,NULL ); }static QEMUMachine prep_machine = {
Signature · 1 line
1.7.3.4
Re: [Qemu-devel] [RFC][PATCH 05/15] prep: replace register_ioport*
Am 06.05.2013 16:26, schrieb Jan Kiszka:
Quoted 6 lines
Convert over to memory regions to obsolete register_ioport*.
Signed-off-by Jan Kiszka <[email protected]>--- hw/ppc/prep.c | 23 +++++++++++++++-------- 1 files changed, 15 insertions(+), 8 deletions(-)
As a heads-up, for PReP we've been preparing to move this System I/O to a qdev/QOM device, so this would likely need rebasing at some point.
Andreas
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
RFC PATCH 6/15 vt82c686: replace register_ioport*
In reply to Jan KiszkaConvert over to memory regions to obsolete register_ioport*.
--- hw/isa/vt82c686.c | 40 ++++++++++++++++++++++++++-------------- 1 files changed, 26 insertions(+), 14 deletions(-)
hw/isa/vt82c686.c +26 -14
@@ -43,10 +43,12 @@ typedef struct SuperIOConfigtypedef struct VT82C686BState {PCIDevice dev; +MemoryRegion superio;SuperIOConfig superio_conf; }VT82C686BState ; -static void superio_ioport_writeb (void *opaque,uint32_t addr,uint32_t data) +static void superio_ioport_writeb (void *opaque,hwaddr addr,uint64_t data, +unsigned size) {int can_write;SuperIOConfig *superio_conf = opaque;
@@ -93,7 +95,7 @@ static void superio_ioport_writeb(void *opaque, uint32_t addr, uint32_t data) } } -static uint32_t superio_ioport_readb (void *opaque,uint32_t addr) +static uint64_t superio_ioport_readb (void *opaque,hwaddr addr,unsigned size) {SuperIOConfig *superio_conf = opaque;
@@ -101,6 +103,16 @@ static uint32_t superio_ioport_readb(void *opaque, uint32_t addr)return (superio_conf->config[superio_conf->index]); } +static const MemoryRegionOps superio_ops = { + .read = superio_ioport_readb, + .write = superio_ioport_writeb, + .endianness =DEVICE_NATIVE_ENDIAN , + .impl = { + .min_access_size =1 , + .max_access_size =1 , + }, +}; +static void vt82c686b_reset (void * opaque) {PCIDevice *d = opaque;
@@ -140,17 +152,7 @@ static void vt82c686b_write_config(PCIDevice * d, uint32_t address,pci_default_write_config (d, address, val, len);if (address ==0x85 ) {/* enable or disable super IO configure */ -if (val &0x2 ) { -/* floppy also uses 0x3f0 and 0x3f1. -* But we do not emulate flopy,so just set it here. */ -isa_unassign_ioport (0x3f0 ,2 ); -register_ioport_read (0x3f0 ,2 ,1 , superio_ioport_readb, - &vt686->superio_conf); -register_ioport_write (0x3f0 ,2 ,1 , superio_ioport_writeb, - &vt686->superio_conf); - }else { -isa_unassign_ioport (0x3f0 ,2 ); - } +memory_region_set_enabled (&vt686->superio, val &0x2 ); } }
@@ -423,11 +425,13 @@ static const VMStateDescription vmstate_via = {/* init the PCI-to-ISA bridge */ static int vt82c686b_initfn (PCIDevice *d) { +VT82C686BState *vt82c =DO_UPCAST (VT82C686BState, dev, d);uint8_t *pci_conf; +ISABus *isa_bus;uint8_t *wmask;int i; -isa_bus_new (&d->qdev,pci_address_space_io (d)); + isa_bus =isa_bus_new (&d->qdev,pci_address_space_io (d)); pci_conf = d->config;pci_config_set_prog_interface (pci_conf,0x0 );
@@ -439,6 +443,14 @@ static int vt82c686b_initfn(PCIDevice *d) } } +memory_region_init_io (&vt82c->superio, &superio_ops, &vt82c->superio_conf, +"superio" ,2 ); +memory_region_set_enabled (&vt82c->superio, false); +/* The floppy also uses 0x3f0 and 0x3f1. +* But we do not emulate a floppy, so just set it here. */ +memory_region_add_subregion (isa_bus->address_space_io,0x3f0 , + &vt82c->superio); +qemu_register_reset (vt82c686b_reset, d);return 0 ;
Signature · 1 line
1.7.3.4
RFC PATCH 7/15 Privatize register_ioport_read/write
In reply to Jan KiszkaNo more users outside of ioport.c.
--- include/exec/ioport.h | 4 ---- ioport.c | 8 ++++---- 2 files changed, 4 insertions(+), 8 deletions(-)
include/exec/ioport.h +0 -4
@@ -39,10 +39,6 @@ typedef uint32_t (IOPortReadFunc)(void *opaque, uint32_t address);typedef void (IOPortDestructor )(void *opaque);void ioport_register (IORange *iorange); -int register_ioport_read (pio_addr_t start,int length,int size, -IOPortReadFunc *func,void *opaque); -int register_ioport_write (pio_addr_t start,int length,int size, -IOPortWriteFunc *func,void *opaque);void isa_unassign_ioport (pio_addr_t start,int length);bool isa_is_ioport_assigned (pio_addr_t start);
ioport.c +4 -4
@@ -139,8 +139,8 @@ static int ioport_bsize(int size, int *bsize) }/* size is the word size in byte */ -int register_ioport_read (pio_addr_t start,int length,int size, -IOPortReadFunc *func,void *opaque) +static int register_ioport_read (pio_addr_t start,int length,int size, +IOPortReadFunc *func,void *opaque) {int i, bsize;
@@ -159,8 +159,8 @@ int register_ioport_read(pio_addr_t start, int length, int size, }/* size is the word size in byte */ -int register_ioport_write (pio_addr_t start,int length,int size, -IOPortWriteFunc *func,void *opaque) +static int register_ioport_write (pio_addr_t start,int length,int size, +IOPortWriteFunc *func,void *opaque) {int i, bsize;
Signature · 1 line
1.7.3.4
RFC PATCH 8/15 isa: implement isa_is_ioport_assigned via memory_region_find
In reply to Jan KiszkaMove isa_is_ioport_assigned to the ISA core and implement it via a memory region lookup. As all IO ports are now directly or indirectly registered via the memory API, this becomes possible and will finally allow us to drop the ioport tables.
--- hw/acpi/piix4.c | 6 +++--- hw/isa/isa-bus.c | 11 +++++++++++ hw/isa/lpc_ich9.c | 8 ++++---- include/exec/ioport.h | 1 - include/hw/isa/isa.h | 2 ++ ioport.c | 7 ------- 6 files changed, 20 insertions(+), 15 deletions(-)
hw/acpi/piix4.c +3 -3
@@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)uint8_t *pci_conf; pci_conf = s->dev.config; - pci_conf[0x5f ] = (isa_is_ioport_assigned (0x378 ) ?0x80 :0 ) |0x10 ; + pci_conf[0x5f ] = (isa_is_ioport_assigned (NULL ,0x378 ) ?0x80 :0 ) |0x10 ; pci_conf[0x63 ] =0x60 ; - pci_conf[0x67 ] = (isa_is_ioport_assigned (0x3f8 ) ?0x08 :0 ) | - (isa_is_ioport_assigned (0x2f8 ) ?0x90 :0 ); + pci_conf[0x67 ] = (isa_is_ioport_assigned (NULL ,0x3f8 ) ?0x08 :0 ) | + (isa_is_ioport_assigned (NULL ,0x2f8 ) ?0x90 :0 ); }
hw/isa/isa-bus.c +11 -0
@@ -279,4 +279,15 @@ MemoryRegion *isa_address_space_io(ISADevice *dev)return isabus->address_space_io; } +bool isa_is_ioport_assigned (ISABus *bus,pio_addr_t start) +{ +if (!bus) { + bus = isabus; + } +if (!bus) { +hw_error ("No isa bus present." ); + } +return memory_region_find (bus->address_space_io, start,1 ).mr !=NULL ; +} +type_init (isabus_register_types)
hw/isa/lpc_ich9.c +4 -4
@@ -480,19 +480,19 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque)uint8_t *pci_conf; pci_conf = s->d.config; -if (isa_is_ioport_assigned (0x3f8 )) { +if (isa_is_ioport_assigned (s->isa_bus,0x3f8 )) {/* com1 */ pci_conf[0x82 ] |=0x01 ; } -if (isa_is_ioport_assigned (0x2f8 )) { +if (isa_is_ioport_assigned (s->isa_bus,0x2f8 )) {/* com2 */ pci_conf[0x82 ] |=0x02 ; } -if (isa_is_ioport_assigned (0x378 )) { +if (isa_is_ioport_assigned (s->isa_bus,0x378 )) {/* lpt */ pci_conf[0x82 ] |=0x04 ; } -if (isa_is_ioport_assigned (0x3f0 )) { +if (isa_is_ioport_assigned (s->isa_bus,0x3f0 )) {/* floppy */ pci_conf[0x82 ] |=0x08 ; }
include/exec/ioport.h +0 -1
@@ -40,7 +40,6 @@ typedef void (IOPortDestructor)(void *opaque);void ioport_register (IORange *iorange);void isa_unassign_ioport (pio_addr_t start,int length); -bool isa_is_ioport_assigned (pio_addr_t start);void cpu_outb (pio_addr_t addr,uint8_t val);void cpu_outw (pio_addr_t addr,uint16_t val);
include/hw/isa/isa.h +2 -0
@@ -80,6 +80,8 @@ void isa_register_portio_list(ISADevice *dev, uint16_t start,const MemoryRegionPortio *portio, void *opaque, const char *name); +bool isa_is_ioport_assigned (ISABus *bus,pio_addr_t start); +static inline ISABus *isa_bus_from_device (ISADevice *d) {return ISA_BUS (qdev_get_parent_bus (DEVICE (d)));
ioport.c +0 -7
@@ -273,13 +273,6 @@ void isa_unassign_ioport(pio_addr_t start, int length) } } -bool isa_is_ioport_assigned (pio_addr_t start) -{ -return (ioport_read_table[0 ][start] || ioport_write_table[0 ][start] || - ioport_read_table[1 ][start] || ioport_write_table[1 ][start] || - ioport_read_table[2 ][start] || ioport_write_table[2 ][start]); -} -/***********************************************************/ void cpu_outb (pio_addr_t addr,uint8_t val)
Signature · 1 line
1.7.3.4
Re: [Qemu-devel] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find
Am 06.05.2013 16:26, schrieb Jan Kiszka:
Quoted 23 lines
Move
isa_is_ioport_assignedto the ISA core and implement it via a memory region lookup. As all IO ports are now directly or indirectly registered via the memory API, this becomes possible and will finally allow us to drop the ioport tables.Signed-off-by Jan Kiszka <[email protected]>--- hw/acpi/piix4.c | 6 +++--- hw/isa/isa-bus.c | 11 +++++++++++ hw/isa/lpc_ich9.c | 8 ++++---- include/exec/ioport.h | 1 - include/hw/isa/isa.h | 2 ++ ioport.c | 7 ------- 6 files changed, 20 insertions(+), 15 deletions(-)hw/acpi/piix4.c +0
-0@@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)uint8_t *pci_conf; pci_conf = s->dev.config;
hw/acpi/piix4.c static void piix4_pm_machine_ready(Notifier *n, void *opaque) · line 395
Quoted 10 lines
hw/acpi/piix4.c +3
-3- pci_conf[0x5f ] = (isa_is_ioport_assigned (0x378 ) ?0x80 :0 ) |0x10 ; + pci_conf[0x5f ] = (isa_is_ioport_assigned (NULL ,0x378 ) ?0x80 :0 ) |0x10 ; pci_conf[0x63 ] =0x60 ; - pci_conf[0x67 ] = (isa_is_ioport_assigned (0x3f8 ) ?0x08 :0 ) | - (isa_is_ioport_assigned (0x2f8 ) ?0x90 :0 ); + pci_conf[0x67 ] = (isa_is_ioport_assigned (NULL ,0x3f8 ) ?0x08 :0 ) | + (isa_is_ioport_assigned (NULL ,0x2f8 ) ?0x90 :0 ); }
Is there really no way to access the ISABus from this device? Would be nice to get rid of global ISA variables and not introduce more dependencies. :)
Andreas
[...]
Quoted 28 lines
hw/isa/lpc_ich9.c +4
-4@@ -480,19 +480,19 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque)uint8_t *pci_conf; pci_conf = s->d.config; -if (isa_is_ioport_assigned (0x3f8 )) { +if (isa_is_ioport_assigned (s->isa_bus,0x3f8 )) {/* com1 */ pci_conf[0x82 ] |=0x01 ; } -if (isa_is_ioport_assigned (0x2f8 )) { +if (isa_is_ioport_assigned (s->isa_bus,0x2f8 )) {/* com2 */ pci_conf[0x82 ] |=0x02 ; } -if (isa_is_ioport_assigned (0x378 )) { +if (isa_is_ioport_assigned (s->isa_bus,0x378 )) {/* lpt */ pci_conf[0x82 ] |=0x04 ; } -if (isa_is_ioport_assigned (0x3f0 )) { +if (isa_is_ioport_assigned (s->isa_bus,0x3f0 )) {/* floppy */ pci_conf[0x82 ] |=0x08 ; }
[snip]
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find
Quoted 32 lines · Andreas Färber
Am 06.05.2013 16:26, schrieb Jan Kiszka:
Quoted 31 lines
Move
isa_is_ioport_assignedto the ISA core and implement it via a memory region lookup. As all IO ports are now directly or indirectly registered via the memory API, this becomes possible and will finally allow us to drop the ioport tables.Signed-off-by Jan Kiszka <[email protected]>--- hw/acpi/piix4.c | 6 +++--- hw/isa/isa-bus.c | 11 +++++++++++ hw/isa/lpc_ich9.c | 8 ++++---- include/exec/ioport.h | 1 - include/hw/isa/isa.h | 2 ++ ioport.c | 7 ------- 6 files changed, 20 insertions(+), 15 deletions(-)hw/acpi/piix4.c +3
-3@@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)uint8_t *pci_conf; pci_conf = s->dev.config; - pci_conf[0x5f ] = (isa_is_ioport_assigned (0x378 ) ?0x80 :0 ) |0x10 ; + pci_conf[0x5f ] = (isa_is_ioport_assigned (NULL ,0x378 ) ?0x80 :0 ) |0x10 ; pci_conf[0x63 ] =0x60 ; - pci_conf[0x67 ] = (isa_is_ioport_assigned (0x3f8 ) ?0x08 :0 ) | - (isa_is_ioport_assigned (0x2f8 ) ?0x90 :0 ); + pci_conf[0x67 ] = (isa_is_ioport_assigned (NULL ,0x3f8 ) ?0x08 :0 ) | + (isa_is_ioport_assigned (NULL ,0x2f8 ) ?0x90 :0 );
Quoted 6 lines
hw/acpi/piix4.c static void piix4_pm_machine_ready(Notifier *n, void *opaque) · line 395
Quoted 2 lines
hw/acpi/piix4.c +0
-0}Is there really no way to access the ISABus from this device? Would be nice to get rid of global ISA variables and not introduce more dependencies. :)
There is likely a way, just didn't find a direct one. So I prefer to limit the scope and leave such cleanups for later. Keep in mind that the existing API had an implicit NULL device, so this is no regression!
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find
In reply to Andreas FärberIl 06/05/2013 16:55, Andreas Färber ha scritto:
Quoted 32 lines
Am 06.05.2013 16:26, schrieb Jan Kiszka:
Quoted 31 lines
Move
isa_is_ioport_assignedto the ISA core and implement it via a memory region lookup. As all IO ports are now directly or indirectly registered via the memory API, this becomes possible and will finally allow us to drop the ioport tables.Signed-off-by Jan Kiszka <[email protected]>--- hw/acpi/piix4.c | 6 +++--- hw/isa/isa-bus.c | 11 +++++++++++ hw/isa/lpc_ich9.c | 8 ++++---- include/exec/ioport.h | 1 - include/hw/isa/isa.h | 2 ++ ioport.c | 7 ------- 6 files changed, 20 insertions(+), 15 deletions(-)hw/acpi/piix4.c +3
-3@@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)uint8_t *pci_conf; pci_conf = s->dev.config; - pci_conf[0x5f ] = (isa_is_ioport_assigned (0x378 ) ?0x80 :0 ) |0x10 ; + pci_conf[0x5f ] = (isa_is_ioport_assigned (NULL ,0x378 ) ?0x80 :0 ) |0x10 ; pci_conf[0x63 ] =0x60 ; - pci_conf[0x67 ] = (isa_is_ioport_assigned (0x3f8 ) ?0x08 :0 ) | - (isa_is_ioport_assigned (0x2f8 ) ?0x90 :0 ); + pci_conf[0x67 ] = (isa_is_ioport_assigned (NULL ,0x3f8 ) ?0x08 :0 ) | + (isa_is_ioport_assigned (NULL ,0x2f8 ) ?0x90 :0 );
Quoted 6 lines
hw/acpi/piix4.c static void piix4_pm_machine_ready(Notifier *n, void *opaque) · line 395
Quoted 2 lines
hw/acpi/piix4.c +0
-0}Is there really no way to access the ISABus from this device? Would be nice to get rid of global ISA variables and not introduce more dependencies. :)
There's always a way to find the ISABus via QOM:
ISABus *isa_bus = (ISABus *) object_resolve_path_type("", TYPE_ISA_BUS, NULL);Paolo
Re: [Qemu-devel] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find
Quoted 40 lines · Paolo Bonzini
Il 06/05/2013 16:55, Andreas Färber ha scritto:
Quoted 32 lines
Am 06.05.2013 16:26, schrieb Jan Kiszka:
Quoted 31 lines
Move
isa_is_ioport_assignedto the ISA core and implement it via a memory region lookup. As all IO ports are now directly or indirectly registered via the memory API, this becomes possible and will finally allow us to drop the ioport tables.Signed-off-by Jan Kiszka <[email protected]>--- hw/acpi/piix4.c | 6 +++--- hw/isa/isa-bus.c | 11 +++++++++++ hw/isa/lpc_ich9.c | 8 ++++---- include/exec/ioport.h | 1 - include/hw/isa/isa.h | 2 ++ ioport.c | 7 ------- 6 files changed, 20 insertions(+), 15 deletions(-)hw/acpi/piix4.c +3
-3@@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)uint8_t *pci_conf; pci_conf = s->dev.config; - pci_conf[0x5f ] = (isa_is_ioport_assigned (0x378 ) ?0x80 :0 ) |0x10 ; + pci_conf[0x5f ] = (isa_is_ioport_assigned (NULL ,0x378 ) ?0x80 :0 ) |0x10 ; pci_conf[0x63 ] =0x60 ; - pci_conf[0x67 ] = (isa_is_ioport_assigned (0x3f8 ) ?0x08 :0 ) | - (isa_is_ioport_assigned (0x2f8 ) ?0x90 :0 ); + pci_conf[0x67 ] = (isa_is_ioport_assigned (NULL ,0x3f8 ) ?0x08 :0 ) | + (isa_is_ioport_assigned (NULL ,0x2f8 ) ?0x90 :0 );Quoted 6 lines
hw/acpi/piix4.c static void piix4_pm_machine_ready(Notifier *n, void *opaque) · line 395
Quoted 2 lines
hw/acpi/piix4.c +0
-0}Is there really no way to access the ISABus from this device? Would be nice to get rid of global ISA variables and not introduce more dependencies. :)
Quoted 3 lines
There's always a way to find the ISABus via QOM:
ISABus *isa_bus = (ISABus *) object_resolve_path_type("", TYPE_ISA_BUS, NULL);
Err, in what way is this better? It also assumes that there is only one.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 08/15] isa: implement isa_is_ioport_assigned via memory_region_find
Il 06/05/2013 17:02, Jan Kiszka ha scritto:
Quoted 41 lines · Paolo Bonzini
Quoted 40 lines
Il 06/05/2013 16:55, Andreas Färber ha scritto:
Quoted 38 lines
Am 06.05.2013 16:26, schrieb Jan Kiszka:
Quoted 23 lines
Move
isa_is_ioport_assignedto the ISA core and implement it via a memory region lookup. As all IO ports are now directly or indirectly registered via the memory API, this becomes possible and will finally allow us to drop the ioport tables.Signed-off-by Jan Kiszka <[email protected]>--- hw/acpi/piix4.c | 6 +++--- hw/isa/isa-bus.c | 11 +++++++++++ hw/isa/lpc_ich9.c | 8 ++++---- include/exec/ioport.h | 1 - include/hw/isa/isa.h | 2 ++ ioport.c | 7 ------- 6 files changed, 20 insertions(+), 15 deletions(-)hw/acpi/piix4.c +0
-0@@ -386,10 +386,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)uint8_t *pci_conf; pci_conf = s->dev.config;hw/acpi/piix4.c static void piix4_pm_machine_ready(Notifier *n, void *opaque) · line 395
Quoted 10 lines
hw/acpi/piix4.c +3
-3- pci_conf[0x5f ] = (isa_is_ioport_assigned (0x378 ) ?0x80 :0 ) |0x10 ; + pci_conf[0x5f ] = (isa_is_ioport_assigned (NULL ,0x378 ) ?0x80 :0 ) |0x10 ; pci_conf[0x63 ] =0x60 ; - pci_conf[0x67 ] = (isa_is_ioport_assigned (0x3f8 ) ?0x08 :0 ) | - (isa_is_ioport_assigned (0x2f8 ) ?0x90 :0 ); + pci_conf[0x67 ] = (isa_is_ioport_assigned (NULL ,0x3f8 ) ?0x08 :0 ) | + (isa_is_ioport_assigned (NULL ,0x2f8 ) ?0x90 :0 ); }Is there really no way to access the ISABus from this device? Would be nice to get rid of global ISA variables and not introduce more dependencies. :)
Quoted 5 lines
Quoted 3 lines
There's always a way to find the ISABus via QOM:
ISABus *isa_bus = (ISABus *) object_resolve_path_type("", TYPE_ISA_BUS, NULL);Err, in what way is this better? It also assumes that there is only one.
I didn't say it is better. :) Unfortunately, the PIIX4 has these register on the "wrong" function, ICH9 fixed it.
You could make this take an AddressSpace instead of an ISABus, and use pci_address_space_io. The assumption then becomes that the ISA and PM devices have the same address spaces, which is somewhat hackish but reasonable.
In fact, with the other new API I have added (address_space_valid), this would become address_space_valid(pci_address_space_io(dev), 0x2f8, 1) and similar. I need to check, but you could remove isa_is_ioport_assigned completely.
Paolo
RFC PATCH 9/15 memory: Introduce address_space_lookup_region
In reply to Jan KiszkaThis new service so far only replaces phys_page_find as public API. In a follow-up step, it will return the effective memory region for the specified address, i.e. after resolving what are currently sub-pages. Moreover, it will also once encapsulate locking and reference counting when we introduce BQL-free dispatching.
--- cputlb.c | 2 +- exec.c | 46 +++++++++++++++++++++------------------------- include/exec/cputlb.h | 2 -- include/exec/memory.h | 9 +++++++++ translate-all.c | 3 +-- 5 files changed, 32 insertions(+), 30 deletions(-)
cputlb.c +1 -1
@@ -254,7 +254,7 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr,if (size !=TARGET_PAGE_SIZE ) {tlb_add_large_page (env, vaddr, size); } - section =phys_page_find (address_space_memory.dispatch, paddr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, paddr);#if defined(DEBUG_TLB ) printf("tlb_set_page: vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx " prot=%x idx=%d pd=0x%08lx\n",
exec.c +21 -25
150 lines not on this page · raw message
include/exec/cputlb.h +0 -2
@@ -26,8 +26,6 @@ void tlb_unprotect_code_phys(CPUArchState *env, ram_addr_t ram_addr, target_ulong vaddr);void tlb_reset_dirty_range (CPUTLBEntry *tlb_entry,uintptr_t start,uintptr_t length); -MemoryRegionSection *phys_page_find (struct AddressSpaceDispatch *d, -hwaddr index);void cpu_tlb_reset_dirty_all (ram_addr_t start1,ram_addr_t length);void tlb_set_dirty (CPUArchState *env,target_ulong vaddr);extern int tlb_flush_count;
include/exec/memory.h +9 -0
@@ -887,6 +887,15 @@ void *address_space_map(AddressSpace *as, hwaddr addr,void address_space_unmap (AddressSpace *as,void *buffer,hwaddr len,int is_write,hwaddr access_len); +/** +* address_space_lookup_region: Looks up memory region corresponding to given +* access address +* +* @as: #AddressSpace to be accessed +* @addr: address within that address space +*/ +MemoryRegionSection *address_space_lookup_region (AddressSpace *as, +hwaddr addr);#endif
translate-all.c +1 -2
@@ -1356,8 +1356,7 @@ void tb_invalidate_phys_addr(hwaddr addr)ram_addr_t ram_addr;MemoryRegionSection *section; - section =phys_page_find (address_space_memory.dispatch, - addr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, addr);if (!(memory_region_is_ram (section->mr) || (section->mr->rom_device && section->mr->readable))) {return ;
Signature · 1 line
1.7.3.4
Re: [Qemu-devel] [RFC][PATCH 09/15] memory: Introduce address_space_lookup_region
Il 06/05/2013 16:26, Jan Kiszka ha scritto:
Quoted 5 lines
This new service so far only replaces
phys_page_findas public API. In a follow-up step, it will return the effective memory region for the specified address, i.e. after resolving what are currently sub-pages. Moreover, it will also once encapsulate locking and reference counting when we introduce BQL-free dispatching.
In my IOMMU rebase I have a similar function:
/* address_space_translate: translate an address range into an address space
* into a MemoryRegionSection and an address range into that section. *
* @as: #AddressSpace to be accessed
* @addr: address within that address space
* @xlat: pointer to address within the returned memory region section's
* #MemoryRegion.
* @len: pointer to length
* @is_write: indicates the transfer direction
*/
MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
hwaddr *xlat, hwaddr *len, bool is_write);
It wraps (actually, replaces) both phys_page_find and memory_region_section_addr.
Paolo
Quoted 237 lines
Signed-off-by Jan Kiszka <[email protected]>--- cputlb.c | 2 +- exec.c | 46 +++++++++++++++++++++------------------------- include/exec/cputlb.h | 2 -- include/exec/memory.h | 9 +++++++++ translate-all.c | 3 +-- 5 files changed, 32 insertions(+), 30 deletions(-)cputlb.c +1
-1@@ -254,7 +254,7 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr,if (size !=TARGET_PAGE_SIZE ) {tlb_add_large_page (env, vaddr, size); } - section =phys_page_find (address_space_memory.dispatch, paddr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, paddr);#if defined(DEBUG_TLB ) printf("tlb_set_page: vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx " prot=%x idx=%d pd=0x%08lx\n",exec.c +21
-25@@ -182,7 +182,8 @@ static void phys_page_set(AddressSpaceDispatch *d,phys_page_set_level (&d->phys_map, &index, &nb, leaf,P_L2_LEVELS -1 ); } -MemoryRegionSection *phys_page_find (AddressSpaceDispatch *d,hwaddr index) +static MemoryRegionSection *phys_page_find (AddressSpaceDispatch *d, +hwaddr index) {PhysPageEntry lp = d->phys_map;PhysPageEntry *p;@@ -1894,19 +1895,16 @@ static void invalidate_and_set_dirty(hwaddr addr,void address_space_rw (AddressSpace *as,hwaddr addr,uint8_t *buf,int len,bool is_write) { -AddressSpaceDispatch *d = as->dispatch;int l;uint8_t *ptr;uint32_t val; -hwaddr page;MemoryRegionSection *section;while (len >0 ) { - page = addr &TARGET_PAGE_MASK ; - l = (page +TARGET_PAGE_SIZE ) - addr; + l = ((addr &TARGET_PAGE_MASK ) +TARGET_PAGE_SIZE ) - addr;if (l > len) l = len; - section =phys_page_find (d, page >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (as, addr);if (is_write) {if (!memory_region_is_ram (section->mr)) {@@ -2006,18 +2004,15 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,void cpu_physical_memory_write_rom (hwaddr addr,const uint8_t *buf,int len) { -AddressSpaceDispatch *d = address_space_memory.dispatch;int l;uint8_t *ptr; -hwaddr page;MemoryRegionSection *section;while (len >0 ) { - page = addr &TARGET_PAGE_MASK ; - l = (page +TARGET_PAGE_SIZE ) - addr; + l = ((addr &TARGET_PAGE_MASK ) +TARGET_PAGE_SIZE ) - addr;if (l > len) l = len; - section =phys_page_find (d, page >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, addr);if (!(memory_region_is_ram (section->mr) ||memory_region_is_romd (section->mr))) {@@ -2096,22 +2091,19 @@ void *address_space_map(AddressSpace *as, hwaddr *plen, bool is_write) { -AddressSpaceDispatch *d = as->dispatch;hwaddr len = *plen;hwaddr todo =0 ;int l; -hwaddr page;MemoryRegionSection *section;ram_addr_t raddr =RAM_ADDR_MAX ;ram_addr_t rlen;void *ret;while (len >0 ) { - page = addr &TARGET_PAGE_MASK ; - l = (page +TARGET_PAGE_SIZE ) - addr; + l = ((addr &TARGET_PAGE_MASK ) +TARGET_PAGE_SIZE ) - addr;if (l > len) l = len; - section =phys_page_find (d, page >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (as, addr);if (!(memory_region_is_ram (section->mr) && !section->readonly)) {if (todo || bounce.buffer) {@@ -2188,6 +2180,11 @@ void cpu_physical_memory_unmap(void *buffer, hwaddr len,return address_space_unmap (&address_space_memory, buffer, len, is_write, access_len); } +MemoryRegionSection *address_space_lookup_region (AddressSpace *as,hwaddr addr) +{ +return phys_page_find (as->dispatch, addr >>TARGET_PAGE_BITS ); +} +/* warning: addr must be aligned */ static inline uint32_t ldl_phys_internal (hwaddr addr,enum device_endian endian)@@ -2196,7 +2193,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,uint32_t val;MemoryRegionSection *section; - section =phys_page_find (address_space_memory.dispatch, addr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, addr);if (!(memory_region_is_ram (section->mr) ||memory_region_is_romd (section->mr))) {@@ -2255,7 +2252,7 @@ static inline uint64_t ldq_phys_internal(hwaddr addr,uint64_t val;MemoryRegionSection *section; - section =phys_page_find (address_space_memory.dispatch, addr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, addr);if (!(memory_region_is_ram (section->mr) ||memory_region_is_romd (section->mr))) {@@ -2322,7 +2319,7 @@ static inline uint32_t lduw_phys_internal(hwaddr addr,uint64_t val;MemoryRegionSection *section; - section =phys_page_find (address_space_memory.dispatch, addr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, addr);if (!(memory_region_is_ram (section->mr) ||memory_region_is_romd (section->mr))) {@@ -2381,7 +2378,7 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)uint8_t *ptr;MemoryRegionSection *section; - section =phys_page_find (address_space_memory.dispatch, addr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, addr);if (!memory_region_is_ram (section->mr) || section->readonly) { addr =memory_region_section_addr (section, addr);@@ -2413,7 +2410,7 @@ void stq_phys_notdirty(hwaddr addr, uint64_t val)uint8_t *ptr;MemoryRegionSection *section; - section =phys_page_find (address_space_memory.dispatch, addr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, addr);if (!memory_region_is_ram (section->mr) || section->readonly) { addr =memory_region_section_addr (section, addr);@@ -2442,7 +2439,7 @@ static inline void stl_phys_internal(hwaddr addr, uint32_t val,uint8_t *ptr;MemoryRegionSection *section; - section =phys_page_find (address_space_memory.dispatch, addr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, addr);if (!memory_region_is_ram (section->mr) || section->readonly) { addr =memory_region_section_addr (section, addr);@@ -2509,7 +2506,7 @@ static inline void stw_phys_internal(hwaddr addr, uint32_t val,uint8_t *ptr;MemoryRegionSection *section; - section =phys_page_find (address_space_memory.dispatch, addr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, addr);if (!memory_region_is_ram (section->mr) || section->readonly) { addr =memory_region_section_addr (section, addr);@@ -2634,8 +2631,7 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr) {MemoryRegionSection *section; - section =phys_page_find (address_space_memory.dispatch, - phys_addr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, phys_addr);return !(memory_region_is_ram (section->mr) ||memory_region_is_romd (section->mr));include/exec/cputlb.h +0
-2@@ -26,8 +26,6 @@ void tlb_unprotect_code_phys(CPUArchState *env, ram_addr_t ram_addr, target_ulong vaddr);void tlb_reset_dirty_range (CPUTLBEntry *tlb_entry,uintptr_t start,uintptr_t length); -MemoryRegionSection *phys_page_find (struct AddressSpaceDispatch *d, -hwaddr index);void cpu_tlb_reset_dirty_all (ram_addr_t start1,ram_addr_t length);void tlb_set_dirty (CPUArchState *env,target_ulong vaddr);extern int tlb_flush_count;include/exec/memory.h +9
-0@@ -887,6 +887,15 @@ void *address_space_map(AddressSpace *as, hwaddr addr,void address_space_unmap (AddressSpace *as,void *buffer,hwaddr len,int is_write,hwaddr access_len); +/** +* address_space_lookup_region: Looks up memory region corresponding to given +* access address +* +* @as: #AddressSpace to be accessed +* @addr: address within that address space +*/ +MemoryRegionSection *address_space_lookup_region (AddressSpace *as, +hwaddr addr);#endif translate-all.c +1
-2@@ -1356,8 +1356,7 @@ void tb_invalidate_phys_addr(hwaddr addr)ram_addr_t ram_addr;MemoryRegionSection *section; - section =phys_page_find (address_space_memory.dispatch, - addr >>TARGET_PAGE_BITS ); + section =address_space_lookup_region (&address_space_memory, addr);if (!(memory_region_is_ram (section->mr) || (section->mr->rom_device && section->mr->readable))) {return ;
Re: [Qemu-devel] [RFC][PATCH 09/15] memory: Introduce address_space_lookup_region
Quoted 23 lines · Paolo Bonzini
Il 06/05/2013 16:26, Jan Kiszka ha scritto:
Quoted 5 lines
This new service so far only replaces
phys_page_findas public API. In a follow-up step, it will return the effective memory region for the specified address, i.e. after resolving what are currently sub-pages. Moreover, it will also once encapsulate locking and reference counting when we introduce BQL-free dispatching.In my IOMMU rebase I have a similar function:
/*
address_space_translate: translate an address range into an address space
* into a MemoryRegionSection and an address range into that section. *
* @as: #AddressSpace to be accessed
* @addr: address within that address space
* @xlat: pointer to address within the returned memory region section's
* #MemoryRegion.
* @len: pointer to length
* @is_write: indicates the transfer direction
*/
MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
hwaddr *xlat, hwaddr *len, boolis_write);
Quoted 2 lines
It wraps (actually, replaces) both
phys_page_findandmemory_region_section_addr.
Good, looks like we are heading in similar directions. What is the purpose of len? When does is_write matter?
In a later step, this should become something like address_space_get_region_ref (to be paired with memory_region_unref, once done). So this one also takes care of incrementing the reference counter or acquiring the BQL, as necessary. Currently, it asks the caller to specify if the BQL is already held, but that will change.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 09/15] memory: Introduce address_space_lookup_region
Il 06/05/2013 16:51, Jan Kiszka ha scritto:
Quoted 24 lines · Paolo Bonzini
Quoted 23 lines
Il 06/05/2013 16:26, Jan Kiszka ha scritto:
Quoted 5 lines
This new service so far only replaces
phys_page_findas public API. In a follow-up step, it will return the effective memory region for the specified address, i.e. after resolving what are currently sub-pages. Moreover, it will also once encapsulate locking and reference counting when we introduce BQL-free dispatching.In my IOMMU rebase I have a similar function:
/*
address_space_translate: translate an address range into an address space
* into a MemoryRegionSection and an address range into that section. *
* @as: #AddressSpace to be accessed
* @addr: address within that address space
* @xlat: pointer to address within the returned memory region section's
* #MemoryRegion.
* @len: pointer to length
* @is_write: indicates the transfer direction
*/
MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
hwaddr *xlat, hwaddr *len, boolis_write);
Quoted 5 lines
Quoted 2 lines
It wraps (actually, replaces) both
phys_page_findandmemory_region_section_addr.Good, looks like we are heading in similar directions. What is the purpose of len? When does
is_writematter?
Both matter when adding the IOMMU. is_write is needed to check for permissions, and len because the translation will be valid for one page only (not for the full size of the MemoryRegion.
I can implement address_space_translate on top of address_space_lookup_region and include your next patch too, so we're fine here.
Paolo
Quoted 8 lines
In a later step, this should become something like
address_space_get_region_ref(to be paired withmemory_region_unref, once done). So this one also takes care of incrementing the reference counter or acquiring the BQL, as necessary. Currently, it asks the caller to specify if the BQL is already held, but that will change.Jan
RFC PATCH 10/15 memory: Rework sub-page handling
In reply to Jan KiszkaSimplify the sub-page handling by implementing it directly in the dispatcher instead of using a redirection memory region. We extend the phys_sections entries to optionally hold a pointer to the sub-section table that used to reside in the subpage_t structure. IOW, we add one optional dispatch level below the existing radix tree.
address_space_lookup_region is extended to take this additional level into account. This direct dispatching to that target memory region will also be helpful when we want to add per-region locking control.
--- exec.c | 175 +++++++++++++++++-------------------------------- include/exec/memory.h | 1 - memory.c | 1 - 3 files changed, 59 insertions(+), 118 deletions(-)
exec.c +59 -116
@@ -51,7 +51,6 @@#include "exec/memory-internal.h" //#define DEBUG_UNASSIGNED -//#define DEBUG_SUBPAGE #if !defined(CONFIG_USER_ONLY )int phys_ram_fd;
@@ -82,7 +81,14 @@ int use_icount;#if !defined(CONFIG_USER_ONLY ) -static MemoryRegionSection *phys_sections; +#define SUBSECTION_IDX (addr) ((addr) & ~TARGET_PAGE_MASK) + +typedef struct PhysSection { +MemoryRegionSection section; +uint16_t *sub_section; +}PhysSection ; + +static PhysSection *phys_sections;static unsigned phys_sections_nb, phys_sections_nb_alloc;static uint16_t phys_section_unassigned;static uint16_t phys_section_notdirty;
@@ -182,8 +188,8 @@ static void phys_page_set(AddressSpaceDispatch *d,phys_page_set_level (&d->phys_map, &index, &nb, leaf,P_L2_LEVELS -1 ); } -static MemoryRegionSection *phys_page_find (AddressSpaceDispatch *d, -hwaddr index) +static PhysSection *phys_section_find (AddressSpaceDispatch *d, +hwaddr index) {PhysPageEntry lp = d->phys_map;PhysPageEntry *p;
@@ -646,7 +652,7 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,and avoid full address decoding in every device. We can't use the high bits of pd for this because IO_MEM_ROMD uses these as a ram address. */ - iotlb = section - phys_sections; + iotlb =container_of (section, PhysSection, section) - phys_sections; iotlb +=memory_region_section_addr (section, paddr); }
@@ -668,27 +674,13 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,#endif /* defined(CONFIG_USER_ONLY) */ #if !defined(CONFIG_USER_ONLY ) +static int subsection_register (PhysSection *psection,uint32_t start, +uint32_t end,uint16_t section); +static void subsections_init (PhysSection *psection); -#define SUBPAGE_IDX (addr) ((addr) & ~TARGET_PAGE_MASK) -typedef struct subpage_t { -MemoryRegion iomem; -hwaddr base; -uint16_t sub_section[TARGET_PAGE_SIZE ]; -}subpage_t ; - -static int subpage_register (subpage_t *mmio,uint32_t start,uint32_t end, -uint16_t section); -static subpage_t *subpage_init (hwaddr base);static void destroy_page_desc (uint16_t section_index) { -MemoryRegionSection *section = &phys_sections[section_index]; -MemoryRegion *mr = section->mr; - -if (mr->subpage) { -subpage_t *subpage =container_of (mr, subpage_t, iomem); -memory_region_destroy (&subpage->iomem); -g_free (subpage); - } +g_free (phys_sections[section_index].sub_section); }static void destroy_l2_mapping (PhysPageEntry *lp,unsigned level)
@@ -722,10 +714,11 @@ static uint16_t phys_section_add(MemoryRegionSection *section) {if (phys_sections_nb == phys_sections_nb_alloc) { phys_sections_nb_alloc =MAX (phys_sections_nb_alloc *2 ,16 ); - phys_sections =g_renew (MemoryRegionSection, phys_sections, + phys_sections =g_renew (PhysSection, phys_sections, phys_sections_nb_alloc); } - phys_sections[phys_sections_nb] = *section; + phys_sections[phys_sections_nb].section = *section; + phys_sections[phys_sections_nb].sub_section =NULL ;return phys_sections_nb++; }
@@ -734,31 +727,31 @@ static void phys_sections_clear(void) phys_sections_nb =0 ; } -static void register_subpage (AddressSpaceDispatch *d,MemoryRegionSection *section) +static void register_subsection (AddressSpaceDispatch *d, +MemoryRegionSection *section) { -subpage_t *subpage;hwaddr base = section->offset_within_address_space &TARGET_PAGE_MASK ; -MemoryRegionSection *existing =phys_page_find (d, base >>TARGET_PAGE_BITS ); +PhysSection *psection =phys_section_find (d, base >>TARGET_PAGE_BITS );MemoryRegionSection subsection = { .offset_within_address_space = base, .size =TARGET_PAGE_SIZE , }; +uint16_t new_section;hwaddr start, end; -assert (existing->mr->subpage || existing->mr == &io_mem_unassigned); +assert (psection->sub_section || + psection->section.mr == &io_mem_unassigned); -if (!(existing->mr->subpage)) { - subpage =subpage_init (base); - subsection.mr = &subpage->iomem; -phys_page_set (d, base >>TARGET_PAGE_BITS ,1 , -phys_section_add (&subsection)); - }else { - subpage =container_of (existing->mr, subpage_t, iomem); +if (!psection->sub_section) { + new_section =phys_section_add (&subsection); + psection = &phys_sections[new_section]; +subsections_init (psection); +phys_page_set (d, base >>TARGET_PAGE_BITS ,1 , new_section); } start = section->offset_within_address_space & ~TARGET_PAGE_MASK ; end = start + section->size -1 ; -subpage_register (subpage, start, end,phys_section_add (section)); +subsection_register (psection, start, end,phys_section_add (section)); }
@@ -786,7 +779,7 @@ static void mem_add(MemoryListener *listener, MemoryRegionSection *section) now.size =MIN (TARGET_PAGE_ALIGN (now.offset_within_address_space) - now.offset_within_address_space, now.size); -register_subpage (d, &now); +register_subsection (d, &now); remain.size -= now.size; remain.offset_within_address_space += now.size; remain.offset_within_region += now.size;
@@ -795,7 +788,7 @@ static void mem_add(MemoryListener *listener, MemoryRegionSection *section) now = remain;if (remain.offset_within_region & ~TARGET_PAGE_MASK ) { now.size =TARGET_PAGE_SIZE ; -register_subpage (d, &now); +register_subsection (d, &now); }else { now.size &=TARGET_PAGE_MASK ;register_multipage (d, &now);
@@ -806,7 +799,7 @@ static void mem_add(MemoryListener *listener, MemoryRegionSection *section) } now = remain;if (now.size) { -register_subpage (d, &now); +register_subsection (d, &now); } }
@@ -1556,49 +1549,6 @@ static const MemoryRegionOps watch_mem_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; -static uint64_t subpage_read (void *opaque,hwaddr addr, -unsigned len) -{ -subpage_t *mmio = opaque; -unsigned int idx =SUBPAGE_IDX (addr); -MemoryRegionSection *section; -#if defined(DEBUG_SUBPAGE ) -printf ("%s: subpage %p len %d addr " TARGET_FMT_plx" idx %d\n" , __func__, - mmio, len, addr, idx); -#endif - - section = &phys_sections[mmio->sub_section[idx]]; - addr += mmio->base; - addr -= section->offset_within_address_space; - addr += section->offset_within_region; -return io_mem_read (section->mr, addr, len); -} - -static void subpage_write (void *opaque,hwaddr addr, -uint64_t value,unsigned len) -{ -subpage_t *mmio = opaque; -unsigned int idx =SUBPAGE_IDX (addr); -MemoryRegionSection *section; -#if defined(DEBUG_SUBPAGE ) -printf ("%s: subpage %p len %d addr " TARGET_FMT_plx -" idx %d value %" PRIx64"\n" , - __func__, mmio, len, addr, idx, value); -#endif - - section = &phys_sections[mmio->sub_section[idx]]; - addr += mmio->base; - addr -= section->offset_within_address_space; - addr += section->offset_within_region; -io_mem_write (section->mr, addr, value, len); -} - -static const MemoryRegionOps subpage_ops = { - .read = subpage_read, - .write = subpage_write, - .endianness =DEVICE_NATIVE_ENDIAN , -}; -static uint64_t subpage_ram_read (void *opaque,hwaddr addr,unsigned size) {
@@ -1631,48 +1581,32 @@ static const MemoryRegionOps subpage_ram_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; -static int subpage_register (subpage_t *mmio,uint32_t start,uint32_t end, -uint16_t section) +static int subsection_register (PhysSection *psection,uint32_t start, +uint32_t end,uint16_t section) {int idx, eidx;if (start >=TARGET_PAGE_SIZE || end >=TARGET_PAGE_SIZE )return -1 ; - idx =SUBPAGE_IDX (start); - eidx =SUBPAGE_IDX (end); -#if defined(DEBUG_SUBPAGE ) -printf ("%s: %p start %08x end %08x idx %08x eidx %08x mem %ld\n" , __func__, - mmio, start, end, idx, eidx, memory); -#endif -if (memory_region_is_ram (phys_sections[section].mr)) { -MemoryRegionSection new_section = phys_sections[section]; + idx =SUBSECTION_IDX (start); + eidx =SUBSECTION_IDX (end); +if (memory_region_is_ram (phys_sections[section].section.mr)) { +MemoryRegionSection new_section = phys_sections[section].section; new_section.mr = &io_mem_subpage_ram; section =phys_section_add (&new_section); }for (; idx <= eidx; idx++) { - mmio->sub_section[idx] = section; + psection->sub_section[idx] = section; }return 0 ; } -static subpage_t *subpage_init (hwaddr base) +static void subsections_init (PhysSection *psection) { -subpage_t *mmio; - - mmio =g_malloc0 (sizeof (subpage_t)); - - mmio->base = base; -memory_region_init_io (&mmio->iomem, &subpage_ops, mmio, -"subpage" ,TARGET_PAGE_SIZE ); - mmio->iomem.subpage = true; -#if defined(DEBUG_SUBPAGE ) -printf ("%s: %p base " TARGET_FMT_plx" len %08x %d\n" , __func__, - mmio, base,TARGET_PAGE_SIZE , subpage_memory); -#endif -subpage_register (mmio,0 ,TARGET_PAGE_SIZE -1 , phys_section_unassigned); - -return mmio; + psection->sub_section =g_malloc0 (sizeof (uint16_t ) *TARGET_PAGE_SIZE ); +subsection_register (psection,0 ,TARGET_PAGE_SIZE -1 , + phys_section_unassigned); }static uint16_t dummy_section (MemoryRegion *mr)
@@ -1689,7 +1623,7 @@ static uint16_t dummy_section(MemoryRegion *mr)MemoryRegion *iotlb_to_region (hwaddr index) { -return phys_sections[index & ~TARGET_PAGE_MASK ].mr; +return phys_sections[index & ~TARGET_PAGE_MASK ].section.mr; }static void io_mem_init (void )
@@ -2182,7 +2116,16 @@ void cpu_physical_memory_unmap(void *buffer, hwaddr len,MemoryRegionSection *address_space_lookup_region (AddressSpace *as,hwaddr addr) { -return phys_page_find (as->dispatch, addr >>TARGET_PAGE_BITS ); +PhysSection *psection; +uint16_t idx; + + psection =phys_section_find (as->dispatch, addr >>TARGET_PAGE_BITS ); +if (psection->sub_section) { + idx = psection->sub_section[SUBSECTION_IDX (addr)]; +return &phys_sections[idx].section; + }else { +return &psection->section; + } }/* warning: addr must be aligned */
@@ -2383,7 +2326,7 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)if (!memory_region_is_ram (section->mr) || section->readonly) { addr =memory_region_section_addr (section, addr);if (memory_region_is_ram (section->mr)) { - section = &phys_sections[phys_section_rom]; + section = &phys_sections[phys_section_rom].section; }io_mem_write (section->mr, addr, val,4 ); }else {
@@ -2415,7 +2358,7 @@ void stq_phys_notdirty(hwaddr addr, uint64_t val)if (!memory_region_is_ram (section->mr) || section->readonly) { addr =memory_region_section_addr (section, addr);if (memory_region_is_ram (section->mr)) { - section = &phys_sections[phys_section_rom]; + section = &phys_sections[phys_section_rom].section; }#ifdef TARGET_WORDS_BIGENDIAN io_mem_write (section->mr, addr, val >>32 ,4 );
@@ -2444,7 +2387,7 @@ static inline void stl_phys_internal(hwaddr addr, uint32_t val,if (!memory_region_is_ram (section->mr) || section->readonly) { addr =memory_region_section_addr (section, addr);if (memory_region_is_ram (section->mr)) { - section = &phys_sections[phys_section_rom]; + section = &phys_sections[phys_section_rom].section; }#if defined(TARGET_WORDS_BIGENDIAN )if (endian ==DEVICE_LITTLE_ENDIAN ) {
@@ -2511,7 +2454,7 @@ static inline void stw_phys_internal(hwaddr addr, uint32_t val,if (!memory_region_is_ram (section->mr) || section->readonly) { addr =memory_region_section_addr (section, addr);if (memory_region_is_ram (section->mr)) { - section = &phys_sections[phys_section_rom]; + section = &phys_sections[phys_section_rom].section; }#if defined(TARGET_WORDS_BIGENDIAN )if (endian ==DEVICE_LITTLE_ENDIAN ) {
include/exec/memory.h +0 -1
@@ -124,7 +124,6 @@ struct MemoryRegion {hwaddr addr;void (*destructor)(MemoryRegion *mr);ram_addr_t ram_addr; -bool subpage;bool terminates;bool readable;bool ram;
memory.c +0 -1
@@ -797,7 +797,6 @@ void memory_region_init(MemoryRegion *mr, mr->size =int128_2_64 (); } mr->addr =0 ; - mr->subpage = false; mr->enabled = true; mr->terminates = false; mr->ram = false;
Signature · 1 line
1.7.3.4
Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling
Il 06/05/2013 16:26, Jan Kiszka ha scritto:
Quoted 11 lines
Simplify the sub-page handling by implementing it directly in the dispatcher instead of using a redirection memory region. We extend the
phys_sectionsentries to optionally hold a pointer to the sub-section table that used to reside in thesubpage_tstructure. IOW, we add one optional dispatch level below the existing radix tree.
address_space_lookup_regionis extended to take this additional level into account. This direct dispatching to that target memory region will also be helpful when we want to add per-region locking control.Signed-off-by Jan Kiszka <[email protected]>
I wonder if subpage_ram is needed at all now. Should be a separate patch anyway, so
Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling
In reply to Jan KiszkaQuoted 9 lines · Jan Kiszka
Simplify the sub-page handling by implementing it directly in the dispatcher instead of using a redirection memory region. We extend the
phys_sectionsentries to optionally hold a pointer to the sub-section table that used to reside in thesubpage_tstructure. IOW, we add one optional dispatch level below the existing radix tree.
address_space_lookup_regionis extended to take this additional level into account. This direct dispatching to that target memory region will also be helpful when we want to add per-region locking control.
This patch seems to break vexpress-a9. Test case if you want it:
http://staging.people.linaro.org/~peter.maydell/vexpress-3.8.tar.gz
(125MB) Edit the 'runme' script to fix up the paths to kernel/initrd/dtb and then run it; before this patch it boots, afterwards it doesn't even manage to start the kernel.
My guess is you've broken subregion-sized mmio regions somehow (and/or regions which are larger than a page in size but start or finish at a non-page-aligned address), and probably in particular the arm_gic regions that a9mpcore maps...
thanks
-- PMM
Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling
Il 06/05/2013 22:46, Peter Maydell ha scritto:
Quoted 11 lines · Jan Kiszka
Quoted 9 lines
Simplify the sub-page handling by implementing it directly in the dispatcher instead of using a redirection memory region. We extend the
phys_sectionsentries to optionally hold a pointer to the sub-section table that used to reside in thesubpage_tstructure. IOW, we add one optional dispatch level below the existing radix tree.
address_space_lookup_regionis extended to take this additional level into account. This direct dispatching to that target memory region will also be helpful when we want to add per-region locking control.
Quoted 5 lines
This patch seems to break vexpress-a9. Test case if you want it:
http://staging.people.linaro.org/~peter.maydell/vexpress-3.8.tar.gz
(125MB) Edit the 'runme' script to fix up the paths tokernel/initrd/dtband then run it; before this patch it boots, afterwards it doesn't even manage to start the kernel.
valgrind is not happy with this patch either:
static int subsection_register(PhysSection *psection, uint32_t start,
uint32_t end, uint16_t section)
{
int idx, eidx;
if (start >= TARGET_PAGE_SIZE || end >= TARGET_PAGE_SIZE)
return -1;
idx = SUBSECTION_IDX(start);
eidx = SUBSECTION_IDX(end);
if (memory_region_is_ram(phys_sections[section].section.mr)) {
MemoryRegionSection new_section = phys_sections[section].section;
new_section.mr = &io_mem_subpage_ram;
section = phys_section_add(&new_section);
}
for (; idx <= eidx; idx++) {
psection->sub_section[idx] = section;
}
return 0;
}The phys_section_add might invalidate psection. If we can drop subpage RAM, that would fix it. But similarly here:
subsection_register(psection, start, end, phys_section_add(section));
The phys_section_add might invalidate psection and the fix is a bit more involved.
Paolo
Quoted 9 lines
My guess is you've broken subregion-sized mmio regions somehow (and/or regions which are larger than a page in size but start or finish at a non-page-aligned address), and probably in particular the
arm_gicregions that a9mpcore maps...thanks
-- PMM
Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling
In reply to Peter MaydellIl 06/05/2013 22:46, Peter Maydell ha scritto:
Quoted 17 lines · Jan Kiszka
Quoted 9 lines
Simplify the sub-page handling by implementing it directly in the dispatcher instead of using a redirection memory region. We extend the
phys_sectionsentries to optionally hold a pointer to the sub-section table that used to reside in thesubpage_tstructure. IOW, we add one optional dispatch level below the existing radix tree.
address_space_lookup_regionis extended to take this additional level into account. This direct dispatching to that target memory region will also be helpful when we want to add per-region locking control.This patch seems to break vexpress-a9. Test case if you want it:
http://staging.people.linaro.org/~peter.maydell/vexpress-3.8.tar.gz
(125MB) Edit the 'runme' script to fix up the paths tokernel/initrd/dtband then run it; before this patch it boots, afterwards it doesn't even manage to start the kernel.
Quoted 4 lines
My guess is you've broken subregion-sized mmio regions somehow (and/or regions which are larger than a page in size but start or finish at a non-page-aligned address), and probably in particular the
arm_gicregions that a9mpcore maps...
I think we just found out what all the subpage stuff is for. :)
When I added address_space_translate(), the trickiest conversion to the new API was in tlb_set_page. Hence I added a "you never know"-style assert:
assert(size >= TARGET_PAGE_SIZE);
if (size != TARGET_PAGE_SIZE) {
tlb_add_large_page(env, vaddr, size);
}- section = phys_page_find(address_space_memory.dispatch,
- paddr >> TARGET_PAGE_BITS);
+ sz = size;
+ section = address_space_translate(&address_space_memory, paddr,
+ &xlat, &sz, false);
+ assert(sz >= TARGET_PAGE_SIZE);
Now, remember that address_space_translate clamps sz on output to the size of the section. And here's what happens:
(gdb) p sz
$4 = 256
(gdb) p *(section->mr)
$5 = {ops = 0x7ffff82d33c0, iommu_ops = 0x0, opaque = 0x7ffff91e6b50, parent = 0x7ffff9069400, owner = 0x0, size = {lo = 256,
hi = 0}, addr = 0, destructor = 0x7ffff7e824d0
<memory_region_destructor_none>, ram_addr = 18446744073709551615, terminates =
true, romd_mode = true, ram = false, readonly = false, enabled =
true, rom_device = false, warning_printed = false,
flush_coalesced_mmio = false, alias = 0x0, alias_offset = 0, priority
= 0, may_overlap = false, subregions = {tqh_first = 0x0,
tqh_last = 0x7ffff91e8ee8}, subregions_link = {tqe_next = 0x0,
tqe_prev = 0x7ffff91e64f8}, coalesced = {tqh_first = 0x0,
tqh_last = 0x7ffff91e8f08}, name = 0x7ffff906bb60 "a9-scu",dirty_log_mask = 0 '\000', ioeventfd_nb = 0, ioeventfds = 0x0,
iommu_target_as = 0x0}
The TLB would only store this region instead of the whole page, and subsequent access past the first 256 bytes would be emulated incorrectly.
For the record, I attach the patch I was using to fix Jan's.
Paolo
Quoted 1 line
From
796abe4e7114d18e74cc869922cc5eb0813396c8Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <[email protected]>
Date: Tue, 7 May 2013 11:30:23 +0200
Subject: [PATCH] subpage fix
Note: this temporarily breaks RAM regions in the I/O address space, but there is none. It will be fixed later when the special address space listener is dropped.
--- exec.c | 56 ++++++++++++-------------------------------------------- 1 files changed, 12 insertions(+), 44 deletions(-)
exec.c +12 -44
@@ -65,7 +65,6 @@ AddressSpace address_space_io;AddressSpace address_space_memory;MemoryRegion io_mem_ram, io_mem_rom, io_mem_unassigned, io_mem_notdirty; -static MemoryRegion io_mem_subpage_ram;#endif
@@ -80,7 +79,8 @@ int use_icount; #if !defined(CONFIG_USER_ONLY) -#define SUBSECTION_IDX(addr) ((addr) & ~TARGET_PAGE_MASK) +#define SUBSECTION_IDX(addr) ((addr) & ~TARGET_PAGE_MASK) +#define PHYS_SECTION_ID(psection) ((psection) - phys_sections) typedef struct PhysSection { MemoryRegionSection section;
@@ -695,7 +695,7 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env, iotlb |= phys_section_rom; } }else { - iotlb =container_of (section, PhysSection, section) - phys_sections; + iotlb =PHYS_SECTION_ID (container_of (section, PhysSection, section)); iotlb += xlat; }
@@ -782,7 +782,7 @@ static void register_subsection(AddressSpaceDispatch *d, .offset_within_address_space = base, .size = TARGET_PAGE_SIZE, }; -uint16_t new_section; +uint16_t new_section, new_subsection;hwaddr start, end; assert(psection->sub_section ||
@@ -793,10 +793,17 @@ static void register_subsection(AddressSpaceDispatch *d, psection = &phys_sections[new_section];subsections_init (psection);phys_page_set (d, base >>TARGET_PAGE_BITS ,1 , new_section); + }else { + new_section =PHYS_SECTION_ID (psection); } + + new_subsection =phys_section_add (section); + +/* phys_section_add invalidates psection, reload it */ + psection = &phys_sections[new_section]; start = section->offset_within_address_space & ~TARGET_PAGE_MASK ; end = start + section->size -1 ; -subsection_register (psection, start, end,phys_section_add (section)); +subsection_register (psection, start, end, new_subsection); }
@@ -1607,38 +1614,6 @@ static const MemoryRegionOps watch_mem_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; -static uint64_t subpage_ram_read (void *opaque,hwaddr addr, -unsigned size) -{ -ram_addr_t raddr = addr; -void *ptr =qemu_get_ram_ptr (raddr); -switch (size) { -case 1 :return ldub_p (ptr); -case 2 :return lduw_p (ptr); -case 4 :return ldl_p (ptr); -default :abort (); - } -} - -static void subpage_ram_write (void *opaque,hwaddr addr, -uint64_t value,unsigned size) -{ -ram_addr_t raddr = addr; -void *ptr =qemu_get_ram_ptr (raddr); -switch (size) { -case 1 :return stb_p (ptr, value); -case 2 :return stw_p (ptr, value); -case 4 :return stl_p (ptr, value); -default :abort (); - } -} - -static const MemoryRegionOps subpage_ram_ops = { - .read = subpage_ram_read, - .write = subpage_ram_write, - .endianness =DEVICE_NATIVE_ENDIAN , -}; -static int subsection_register (PhysSection *psection,uint32_t start,uint32_t end,uint16_t section) {
@@ -1648,11 +1623,6 @@ static int subsection_register(PhysSection *psection, uint32_t start,return -1 ; idx =SUBSECTION_IDX (start); eidx =SUBSECTION_IDX (end); -if (memory_region_is_ram (phys_sections[section].section.mr)) { -MemoryRegionSection new_section = phys_sections[section].section; - new_section.mr = &io_mem_subpage_ram; - section =phys_section_add (&new_section); - }for (; idx <= eidx; idx++) { psection->sub_section[idx] = section; }
@@ -1692,8 +1662,6 @@ static void io_mem_init(void) "unassigned", UINT64_MAX);memory_region_init_io (&io_mem_notdirty, ¬dirty_mem_ops,NULL ,"notdirty" ,UINT64_MAX ); -memory_region_init_io (&io_mem_subpage_ram, &subpage_ram_ops,NULL , -"subpage-ram" ,UINT64_MAX );memory_region_init_io (&io_mem_watch, &watch_mem_ops,NULL ,"watch" ,UINT64_MAX ); }
Signature · 1 line
1.7.1
Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling
Quoted 55 lines · Paolo Bonzini
Il 06/05/2013 22:46, Peter Maydell ha scritto:
Quoted 17 lines · Jan Kiszka
Quoted 9 lines
Simplify the sub-page handling by implementing it directly in the dispatcher instead of using a redirection memory region. We extend the
phys_sectionsentries to optionally hold a pointer to the sub-section table that used to reside in thesubpage_tstructure. IOW, we add one optional dispatch level below the existing radix tree.
address_space_lookup_regionis extended to take this additional level into account. This direct dispatching to that target memory region will also be helpful when we want to add per-region locking control.This patch seems to break vexpress-a9. Test case if you want it:
http://staging.people.linaro.org/~peter.maydell/vexpress-3.8.tar.gz
(125MB) Edit the 'runme' script to fix up the paths tokernel/initrd/dtband then run it; before this patch it boots, afterwards it doesn't even manage to start the kernel.Quoted 4 lines
My guess is you've broken subregion-sized mmio regions somehow (and/or regions which are larger than a page in size but start or finish at a non-page-aligned address), and probably in particular the
arm_gicregions that a9mpcore maps...I think we just found out what all the subpage stuff is for. :)
When I added
address_space_translate(), the trickiest conversion to the new API was intlb_set_page. Hence I added a "you never know"-style assert:assert(size >= TARGET_PAGE_SIZE); if (size != TARGET_PAGE_SIZE) { tlb_add_large_page(env, vaddr, size); }(quoted diff) +4
-2- section = phys_page_find(address_space_memory.dispatch, - paddr >> TARGET_PAGE_BITS); + sz = size; + section = address_space_translate(&address_space_memory, paddr, + &xlat, &sz, false); + assert(sz >= TARGET_PAGE_SIZE);Now, remember that
address_space_translateclamps sz on output to the size of the section. And here's what happens:(gdb) p sz
$4 = 256
(gdb) p *(section->mr)
$5 = {ops =0x7ffff82d33c0,iommu_ops=0x0, opaque =0x7ffff91e6b50, parent =0x7ffff9069400, owner =0x0, size = {lo = 256,hi = 0}, addr = 0, destructor = 0x7ffff7e824d0<
memory_region_destructor_none>,ram_addr= 18446744073709551615, terminates =
true,romd_mode= true, ram = false, readonly = false, enabled =
true,rom_device= false,warning_printed= false,
flush_coalesced_mmio= false, alias =0x0,alias_offset= 0, priority
= 0,may_overlap= false, subregions = {tqh_first=0x0,
Quoted 8 lines
tqh_last = 0x7ffff91e8ee8}, subregions_link = {tqe_next = 0x0, tqe_prev = 0x7ffff91e64f8}, coalesced = {tqh_first = 0x0, tqh_last = 0x7ffff91e8f08}, name = 0x7ffff906bb60 "a9-scu",
dirty_log_mask= 0 '\000',ioeventfd_nb= 0, ioeventfds =0x0,
iommu_target_as=0x0}The TLB would only store this region instead of the whole page, and subsequent access past the first 256 bytes would be emulated incorrectly.
Good catch. Hmm, and a tricky issue. So we need to preserve the dispatching read/write handler of sub-pages, and a reference to the sub-page would continue to end up in the TCG TLBs. But reference counting of the memory regions that page may point to becomes hairy.
Conceptually, we would have to increment the counter for all regions a sub-page refers to. Even worse, regions may be added to the sub-page while it is referenced by some user. Doesn't work.
Well, the alternative is to handle a sub-page dispatch (ie. calling into subpage_[ram_]read/write just like address_space_rw: take the necessary lock that protect mapping changes, look into the sub-page and pick up the target region, invoke memory_region_ref on it, perform the access and unref the region again. Slow, but that's how sub-pages are. And it only affects TCG. Hmm, or does your IOMMU core cache translations on a per-page base as well?
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling
Quoted 7 lines · Jan Kiszka
Well, the alternative is to handle a sub-page dispatch (ie. calling into subpage_[ram_]read/write just like
address_space_rw: take the necessary lock that protect mapping changes, look into the sub-page and pick up the target region, invokememory_region_refon it, perform the access and unref the region again. Slow, but that's how sub-pages are. And it only affects TCG. Hmm, or does your IOMMU core cache translations on a per-page base as well?
OK, there is no translation caching in the memory core. So I will preserve the dispatching functions of sub-pages, just like the term "sub-page" - along with a comment why we depend on page granularity.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 10/15] memory: Rework sub-page handling
Quoted 9 lines · Jan Kiszka
Quoted 7 lines
Well, the alternative is to handle a sub-page dispatch (ie. calling into subpage_[ram_]read/write just like
address_space_rw: take the necessary lock that protect mapping changes, look into the sub-page and pick up the target region, invokememory_region_refon it, perform the access and unref the region again. Slow, but that's how sub-pages are. And it only affects TCG. Hmm, or does your IOMMU core cache translations on a per-page base as well?
Quoted 3 lines
OK, there is no translation caching in the memory core. So I will preserve the dispatching functions of sub-pages, just like the term "sub-page" - along with a comment why we depend on page granularity.
Note that TCG will cache translations because the TLB entry is filled with the page after translation has taken place. In fact, this is the main change from Avi's series to mine, and a side-effect of centralizing the translation in address_space_translate.
It shouldn't be a problem though, the TLB entry will point to the subpage and the compiled code will dispatch to it.
Paolo
RFC PATCH 11/15 memory: Allow unaligned address_space_rw
In reply to Jan KiszkaThis will be needed for some corner cases with para-virtual the I/O ports.
--- exec.c | 33 ++++++++++++++++++--------------- 1 files changed, 18 insertions(+), 15 deletions(-)
exec.c +18 -15
@@ -1833,38 +1833,41 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,uint8_t *ptr;uint32_t val;MemoryRegionSection *section; +MemoryRegion *mr;while (len >0 ) { l = ((addr &TARGET_PAGE_MASK ) +TARGET_PAGE_SIZE ) - addr;if (l > len) l = len; section =address_space_lookup_region (as, addr); + mr = section->mr;if (is_write) { -if (!memory_region_is_ram (section->mr)) { +if (!memory_region_is_ram (mr)) {hwaddr addr1; addr1 =memory_region_section_addr (section, addr);/* XXX: could force cpu_single_env to NULL to avoid potential bugs */ -if (l >=4 && ((addr1 &3 ) ==0 )) { +if (l >=4 && ((addr1 &3 ) ==0 || mr->ops->impl.unaligned)) {/* 32 bit write access */ val =ldl_p (buf); -io_mem_write (section->mr, addr1, val,4 ); +io_mem_write (mr, addr1, val,4 ); l =4 ; - }else if (l >=2 && ((addr1 &1 ) ==0 )) { + }else if (l >=2 && + ((addr1 &1 ) ==0 || mr->ops->impl.unaligned)) {/* 16 bit write access */ val =lduw_p (buf); -io_mem_write (section->mr, addr1, val,2 ); +io_mem_write (mr, addr1, val,2 ); l =2 ; }else {/* 8 bit write access */ val =ldub_p (buf); -io_mem_write (section->mr, addr1, val,1 ); +io_mem_write (mr, addr1, val,1 ); l =1 ; } }else if (!section->readonly) {ram_addr_t addr1; - addr1 =memory_region_get_ram_addr (section->mr) + addr1 =memory_region_get_ram_addr (mr) +memory_region_section_addr (section, addr);/* RAM case */ ptr =qemu_get_ram_ptr (addr1);
@@ -1873,30 +1876,30 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,qemu_put_ram_ptr (ptr); } }else { -if (!(memory_region_is_ram (section->mr) || -memory_region_is_romd (section->mr))) { +if (!(memory_region_is_ram (mr) ||memory_region_is_romd (mr))) {hwaddr addr1;/* I/O case */ addr1 =memory_region_section_addr (section, addr); -if (l >=4 && ((addr1 &3 ) ==0 )) { +if (l >=4 && ((addr1 &3 ) ==0 || mr->ops->impl.unaligned)) {/* 32 bit read access */ - val =io_mem_read (section->mr, addr1,4 ); + val =io_mem_read (mr, addr1,4 );stl_p (buf, val); l =4 ; - }else if (l >=2 && ((addr1 &1 ) ==0 )) { + }else if (l >=2 && + ((addr1 &1 ) ==0 || mr->ops->impl.unaligned)) {/* 16 bit read access */ - val =io_mem_read (section->mr, addr1,2 ); + val =io_mem_read (mr, addr1,2 );stw_p (buf, val); l =2 ; }else {/* 8 bit read access */ - val =io_mem_read (section->mr, addr1,1 ); + val =io_mem_read (mr, addr1,1 );stb_p (buf, val); l =1 ; } }else {/* RAM case */ - ptr =qemu_get_ram_ptr (section->mr->ram_addr + ptr =qemu_get_ram_ptr (mr->ram_addr +memory_region_section_addr (section, addr));memcpy (buf, ptr, l);
Signature · 1 line
1.7.3.4
Re: [Qemu-devel] [RFC][PATCH 11/15] memory: Allow unaligned address_space_rw
Il 06/05/2013 16:26, Jan Kiszka ha scritto:
Quoted 24 lines
This will be needed for some corner cases with para-virtual the I/O ports.
Signed-off-by Jan Kiszka <[email protected]>--- exec.c | 33 ++++++++++++++++++--------------- 1 files changed, 18 insertions(+), 15 deletions(-)exec.c +2
-0@@ -1833,38 +1833,41 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,uint8_t *ptr;uint32_t val;MemoryRegionSection *section; +MemoryRegion *mr;while (len >0 ) { l = ((addr &TARGET_PAGE_MASK ) +TARGET_PAGE_SIZE ) - addr;if (l > len) l = len; section =address_space_lookup_region (as, addr); + mr = section->mr;
exec.c void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf, · line 1851
Quoted 10 lines
exec.c +2
-2if (is_write) { -if (!memory_region_is_ram (section->mr)) { +if (!memory_region_is_ram (mr)) {hwaddr addr1; addr1 =memory_region_section_addr (section, addr);/* XXX: could force cpu_single_env to NULL to avoid potential bugs */ -if (l >=4 && ((addr1 &3 ) ==0 )) { +if (l >=4 && ((addr1 &3 ) ==0 || mr->ops->impl.unaligned)) {
Does the length matter at all if unaligned accesses are allowed? I think it shouldn't...
Paolo
Quoted 67 lines
exec.c +14
-13/* 32 bit write access */ val =ldl_p (buf); -io_mem_write (section->mr, addr1, val,4 ); +io_mem_write (mr, addr1, val,4 ); l =4 ; - }else if (l >=2 && ((addr1 &1 ) ==0 )) { + }else if (l >=2 && + ((addr1 &1 ) ==0 || mr->ops->impl.unaligned)) {/* 16 bit write access */ val =lduw_p (buf); -io_mem_write (section->mr, addr1, val,2 ); +io_mem_write (mr, addr1, val,2 ); l =2 ; }else {/* 8 bit write access */ val =ldub_p (buf); -io_mem_write (section->mr, addr1, val,1 ); +io_mem_write (mr, addr1, val,1 ); l =1 ; } }else if (!section->readonly) {ram_addr_t addr1; - addr1 =memory_region_get_ram_addr (section->mr) + addr1 =memory_region_get_ram_addr (mr) +memory_region_section_addr (section, addr);/* RAM case */ ptr =qemu_get_ram_ptr (addr1);@@ -1873,30 +1876,30 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,qemu_put_ram_ptr (ptr); } }else { -if (!(memory_region_is_ram (section->mr) || -memory_region_is_romd (section->mr))) { +if (!(memory_region_is_ram (mr) ||memory_region_is_romd (mr))) {hwaddr addr1;/* I/O case */ addr1 =memory_region_section_addr (section, addr); -if (l >=4 && ((addr1 &3 ) ==0 )) { +if (l >=4 && ((addr1 &3 ) ==0 || mr->ops->impl.unaligned)) {/* 32 bit read access */ - val =io_mem_read (section->mr, addr1,4 ); + val =io_mem_read (mr, addr1,4 );stl_p (buf, val); l =4 ; - }else if (l >=2 && ((addr1 &1 ) ==0 )) { + }else if (l >=2 && + ((addr1 &1 ) ==0 || mr->ops->impl.unaligned)) {/* 16 bit read access */ - val =io_mem_read (section->mr, addr1,2 ); + val =io_mem_read (mr, addr1,2 );stw_p (buf, val); l =2 ; }else {/* 8 bit read access */ - val =io_mem_read (section->mr, addr1,1 ); + val =io_mem_read (mr, addr1,1 );stb_p (buf, val); l =1 ; } }else {/* RAM case */ - ptr =qemu_get_ram_ptr (section->mr->ram_addr + ptr =qemu_get_ram_ptr (mr->ram_addr +memory_region_section_addr (section, addr));memcpy (buf, ptr, l);
Re: [Qemu-devel] [RFC][PATCH 11/15] memory: Allow unaligned address_space_rw
Quoted 26 lines · Paolo Bonzini
Il 06/05/2013 16:26, Jan Kiszka ha scritto:
Quoted 25 lines
This will be needed for some corner cases with para-virtual the I/O ports.
Signed-off-by Jan Kiszka <[email protected]>--- exec.c | 33 ++++++++++++++++++--------------- 1 files changed, 18 insertions(+), 15 deletions(-)exec.c +2
-0@@ -1833,38 +1833,41 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,uint8_t *ptr;uint32_t val;MemoryRegionSection *section; +MemoryRegion *mr;while (len >0 ) { l = ((addr &TARGET_PAGE_MASK ) +TARGET_PAGE_SIZE ) - addr;if (l > len) l = len; section =address_space_lookup_region (as, addr); + mr = section->mr;
Quoted 12 lines
exec.c void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf, · line 1851
Quoted 9 lines
exec.c +2
-2if (is_write) { -if (!memory_region_is_ram (section->mr)) { +if (!memory_region_is_ram (mr)) {hwaddr addr1; addr1 =memory_region_section_addr (section, addr);/* XXX: could force cpu_single_env to NULL to avoid potential bugs */ -if (l >=4 && ((addr1 &3 ) ==0 )) { +if (l >=4 && ((addr1 &3 ) ==0 || mr->ops->impl.unaligned)) {Does the length matter at all if unaligned accesses are allowed? I think it shouldn't...
What do you mean? The length test here is not about alignment, it's about proper split-up depending on the input size (we cannot use 32 bit for all accesses and do not want to make them all byte accesses, do we?).
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 11/15] memory: Allow unaligned address_space_rw
Il 06/05/2013 16:58, Jan Kiszka ha scritto:
Quoted 40 lines · Paolo Bonzini
Quoted 26 lines
Il 06/05/2013 16:26, Jan Kiszka ha scritto:
Quoted 25 lines
This will be needed for some corner cases with para-virtual the I/O ports.
Signed-off-by Jan Kiszka <[email protected]>--- exec.c | 33 ++++++++++++++++++--------------- 1 files changed, 18 insertions(+), 15 deletions(-)exec.c +2
-0@@ -1833,38 +1833,41 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,uint8_t *ptr;uint32_t val;MemoryRegionSection *section; +MemoryRegion *mr;while (len >0 ) { l = ((addr &TARGET_PAGE_MASK ) +TARGET_PAGE_SIZE ) - addr;if (l > len) l = len; section =address_space_lookup_region (as, addr); + mr = section->mr;Quoted 12 lines
exec.c void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf, · line 1851
Quoted 9 lines
exec.c +2
-2if (is_write) { -if (!memory_region_is_ram (section->mr)) { +if (!memory_region_is_ram (mr)) {hwaddr addr1; addr1 =memory_region_section_addr (section, addr);/* XXX: could force cpu_single_env to NULL to avoid potential bugs */ -if (l >=4 && ((addr1 &3 ) ==0 )) { +if (l >=4 && ((addr1 &3 ) ==0 || mr->ops->impl.unaligned)) {Does the length matter at all if unaligned accesses are allowed? I think it shouldn't...
Quoted 3 lines
What do you mean? The length test here is not about alignment, it's about proper split-up depending on the input size (we cannot use 32 bit for all accesses and do not want to make them all byte accesses, do we?).
Oh right, I was thinking of my IOMMU tree where I have:
l = len;
section = address_space_translate(as, addr, &addr1, &l, is_write);
if (is_write) {
if (!memory_region_is_ram(section->mr)) {
/* XXX: could force cpu_single_env to NULL to avoid
potential bugs */
if (l >= 4 && ((addr1 & 3) == 0)) {and address_space_translate does:
*plen = MIN(section->size - addr, *plen);
It should not do this if section->mr->ops.unaligned. Whoever rebases on top of the other should keep this in mind.
Paolo
RFC PATCH 12/15 vmware-vga: Accept unaligned I/O accesses
In reply to Jan KiszkaBefore switching to the memory core dispatcher, we need to make sure that this pv-device will continue to receive unaligned portio accesses.
--- hw/display/vmware_vga.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
hw/display/vmware_vga.c +4 -0
@@ -1241,6 +1241,10 @@ static const MemoryRegionOps vmsvga_io_ops = { .valid = { .min_access_size = 4, .max_access_size = 4, + .unaligned = true, + }, + .impl = { + .unaligned = true, }, };
Signature · 1 line
1.7.3.4
Re: [Qemu-devel] [RFC][PATCH 12/15] vmware-vga: Accept unaligned I/O accesses
Il 06/05/2013 16:26, Jan Kiszka ha scritto:
Quoted 14 lines
Before switching to the memory core dispatcher, we need to make sure that this pv-device will continue to receive unaligned portio accesses.
Signed-off-by Jan Kiszka <[email protected]>--- hw/display/vmware_vga.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)hw/display/vmware_vga.c +0
-0
hw/display/vmware_vga.c static const MemoryRegionOps vmsvga_io_ops = { · line 1250
Quoted 10 lines
hw/display/vmware_vga.c +4
-0@@ -1241,6 +1241,10 @@ static const MemoryRegionOps vmsvga_io_ops = { .valid = { .min_access_size = 4, .max_access_size = 4, + .unaligned = true, + }, + .impl = { + .unaligned = true, }, };
The Xen platform device needs the same.
Paolo
Re: [Qemu-devel] [RFC][PATCH 12/15] vmware-vga: Accept unaligned I/O accesses
Quoted 19 lines · Paolo Bonzini
Il 06/05/2013 16:26, Jan Kiszka ha scritto:
Quoted 18 lines
Before switching to the memory core dispatcher, we need to make sure that this pv-device will continue to receive unaligned portio accesses.
Signed-off-by Jan Kiszka <[email protected]>--- hw/display/vmware_vga.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)hw/display/vmware_vga.c +2
-0@@ -1241,6 +1241,10 @@ static const MemoryRegionOps vmsvga_io_ops = { .valid = { .min_access_size = 4, .max_access_size = 4, + .unaligned = true, + },
Quoted 8 lines
hw/display/vmware_vga.c static const MemoryRegionOps vmsvga_io_ops = { · line 1250
Quoted 6 lines
hw/display/vmware_vga.c +2
-0+ .impl = { + .unaligned = true, }, };The Xen platform device needs the same.
OK, good to know. In theory, this should only affect weird PV, so I hope that is all.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
RFC PATCH 13/15 ioport: Switch dispatching to memory core layer
In reply to Jan KiszkaThe current ioport dispatcher is a complex beast, mostly due to the need to deal with old portio interface users. But we can overcome it without converting all portio users by embedding the required base address of a MemoryRegionPortio access into that data structure. That removes the need to have the additional MemoryRegionIORange structure in the loop on every access.
To handle old portio memory ops, we simply hook memory_region_iorange_read/write into the normal dispatch handler, calling them when standard region handler is not set, but old_portio is.
We can drop the additional aliasing of ioport regions and can also the special address space listener. cpu_in/out now simply call into address_space_read/write.
--- exec.c | 25 ---------- include/exec/ioport.h | 1 - include/exec/memory-internal.h | 2 - include/exec/memory.h | 3 +- ioport.c | 49 +++++++++++-------- memory.c | 101 ++++++++++++++------------------------- 6 files changed, 66 insertions(+), 115 deletions(-)
exec.c +0 -25
@@ -1679,24 +1679,6 @@ static void core_log_global_stop(MemoryListener *listener)cpu_physical_memory_set_dirty_tracking (0 ); } -static void io_region_add (MemoryListener *listener, -MemoryRegionSection *section) -{ -MemoryRegionIORange *mrio =g_new (MemoryRegionIORange,1 ); - - mrio->mr = section->mr; - mrio->offset = section->offset_within_region; -iorange_init (&mrio->iorange, &memory_region_iorange_ops, - section->offset_within_address_space, section->size); -ioport_register (&mrio->iorange); -} - -static void io_region_del (MemoryListener *listener, -MemoryRegionSection *section) -{ -isa_unassign_ioport (section->offset_within_address_space, section->size); -} -static MemoryListener core_memory_listener = { .begin = core_begin, .log_global_start = core_log_global_start,
@@ -1704,12 +1686,6 @@ static MemoryListener core_memory_listener = { .priority = 1, }; -static MemoryListener io_memory_listener = { - .region_add = io_region_add, - .region_del = io_region_del, - .priority =0 , -}; -static MemoryListener tcg_memory_listener = { .commit = tcg_commit, };
@@ -1752,7 +1728,6 @@ static void memory_map_init(void) address_space_io.name ="I/O" ;memory_listener_register (&core_memory_listener, &address_space_memory); -memory_listener_register (&io_memory_listener, &address_space_io);memory_listener_register (&tcg_memory_listener, &address_space_memory); dma_context_init(&dma_context_memory, &address_space_memory,
include/exec/ioport.h +0 -1
@@ -56,7 +56,6 @@ typedef struct PortioList {struct MemoryRegion *address_space;unsigned nr;struct MemoryRegion **regions; -struct MemoryRegion **aliases;void *opaque;const char *name; } PortioList;
include/exec/memory-internal.h +0 -2
@@ -128,8 +128,6 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,void cpu_physical_memory_reset_dirty (ram_addr_t start,ram_addr_t end,int dirty_flags); -extern const IORangeOps memory_region_iorange_ops; -#endif #endif
include/exec/memory.h +2 -1
@@ -151,6 +151,7 @@ struct MemoryRegionPortio {unsigned size;IOPortReadFunc *read;IOPortWriteFunc *write; +uint32_t base;/* private field */ };#define PORTIO_END_OF_LIST () { }
@@ -810,7 +811,7 @@ void mtree_info(fprintf_function mon_printf, void *f);* address_space_init: initializes an address space * * @as: an uninitialized #AddressSpace -* @root: a #MemoryRegion that routes addesses for the address space +* @root: a #MemoryRegion that routes addresses for the address space */ void address_space_init (AddressSpace *as,MemoryRegion *root);
ioport.c +28 -21
142 lines not on this page · raw message
memory.c +36 -65
129 lines not on this page · raw message
Signature · 1 line
1.7.3.4
RFC PATCH 14/15 ioport: Remove unused old dispatching services
In reply to Jan KiszkaRemove unused ioport_register and isa_unassign_ioport along with everything that only those services used.
--- include/exec/ioport.h | 5 - include/exec/iorange.h | 31 ------ include/exec/memory.h | 9 -- ioport.c | 238 ------------------------------------------------ 4 files changed, 0 insertions(+), 283 deletions(-) delete mode 100644 include/exec/iorange.h
include/exec/ioport.h +0 -5
@@ -25,7 +25,6 @@#define IOPORT_H #include "qemu-common.h" -#include "exec/iorange.h" typedef uint32_t pio_addr_t ;#define FMT_pioaddr PRIx32
@@ -36,10 +35,6 @@ typedef uint32_t pio_addr_t;/* These should really be in isa.h, but are here to make pc.h happy. */ typedef void (IOPortWriteFunc )(void *opaque,uint32_t address,uint32_t data);typedef uint32_t (IOPortReadFunc )(void *opaque,uint32_t address); -typedef void (IOPortDestructor )(void *opaque); - -void ioport_register (IORange *iorange); -void isa_unassign_ioport (pio_addr_t start,int length);void cpu_outb (pio_addr_t addr,uint8_t val);void cpu_outw (pio_addr_t addr,uint16_t val);
include/exec/iorange.h deleted +0 -31
@@ -1,31 +0,0 @@ -#ifndef IORANGE_H -#define IORANGE_H - -#include <stdint.h> - -typedef struct IORange IORange ; -typedef struct IORangeOps IORangeOps ; - -struct IORangeOps { -void (*read)(IORange *iorange,uint64_t offset,unsigned width, -uint64_t *data); -void (*write)(IORange *iorange,uint64_t offset,unsigned width, -uint64_t data); -void (*destructor)(IORange *iorange); -}; - -struct IORange { -const IORangeOps *ops; -uint64_t base; -uint64_t len; -}; - -static inline void iorange_init (IORange *iorange,const IORangeOps *ops, -uint64_t base,uint64_t len) -{ - iorange->ops = ops; - iorange->base = base; - iorange->len = len; -} - -#endif
include/exec/memory.h +0 -9
@@ -22,7 +22,6 @@#include "exec/cpu-common.h" #include "exec/hwaddr.h" #include "qemu/queue.h" -#include "exec/iorange.h" #include "exec/ioport.h" #include "qemu/int128.h"
@@ -42,14 +41,6 @@ struct MemoryRegionMmio {CPUWriteMemoryFunc *write[3 ]; }; -/* Internal use; thunks between old-style IORange and MemoryRegions. */ -typedef struct MemoryRegionIORange MemoryRegionIORange ; -struct MemoryRegionIORange { -IORange iorange; -MemoryRegion *mr; -hwaddr offset; -}; -/* * Memory region callbacks */
ioport.c +0 -238
252 lines not on this page · raw message
Signature · 1 line
1.7.3.4
RFC PATCH 15/15 ioport: Move IOPortRead/WriteFunc typedefs to memory.h
In reply to Jan KiszkaMove the function types required for MemoryRegionPortio to memory.h. This allows to let ioport.h depend on memory.h, which is more consistent instead than the other way around.
--- include/exec/ioport.h | 8 +------- include/exec/memory.h | 4 +++- 2 files changed, 4 insertions(+), 8 deletions(-)
include/exec/ioport.h +1 -7
@@ -25,6 +25,7 @@#define IOPORT_H #include "qemu-common.h" +#include "exec/memory.h" typedef uint32_t pio_addr_t ;#define FMT_pioaddr PRIx32
@@ -32,10 +33,6 @@ typedef uint32_t pio_addr_t;#define MAX_IOPORTS (64 * 1024)#define IOPORTS_MASK (MAX_IOPORTS - 1) -/* These should really be in isa.h, but are here to make pc.h happy. */ -typedef void (IOPortWriteFunc )(void *opaque,uint32_t address,uint32_t data); -typedef uint32_t (IOPortReadFunc )(void *opaque,uint32_t address); -void cpu_outb (pio_addr_t addr,uint8_t val);void cpu_outw (pio_addr_t addr,uint16_t val);void cpu_outl (pio_addr_t addr,uint32_t val);
@@ -43,9 +40,6 @@ uint8_t cpu_inb(pio_addr_t addr);uint16_t cpu_inw (pio_addr_t addr);uint32_t cpu_inl (pio_addr_t addr); -struct MemoryRegion ; -struct MemoryRegionPortio ; - typedef struct PortioList { const struct MemoryRegionPortio *ports; struct MemoryRegion *address_space;
include/exec/memory.h +3 -1
@@ -22,7 +22,6 @@#include "exec/cpu-common.h" #include "exec/hwaddr.h" #include "qemu/queue.h" -#include "exec/ioport.h" #include "qemu/int128.h" typedef struct MemoryRegionOps MemoryRegionOps ;
@@ -136,6 +135,9 @@ struct MemoryRegion {MemoryRegionIoeventfd *ioeventfds; }; +typedef void (IOPortWriteFunc )(void *opaque,uint32_t address,uint32_t data); +typedef uint32_t (IOPortReadFunc )(void *opaque,uint32_t address); +struct MemoryRegionPortio {uint32_t offset;uint32_t len;
Signature · 1 line
1.7.3.4
Am 06.05.2013 16:26, schrieb Jan Kiszka:
Quoted 2 lines
This series converts the remaining users of register_ioport* to portio lists,
Why does it need to be lists? Is there anything wrong with using isa_register_ioport() as done in Hervé's previous ioport cleanup series? (Hope you were aware of those ppc-centered discussions?)
Andreas
Quoted 55 lines
simplifies the handling of subpages and adds support for unaligned memory region accesses. Then it replaces the current portio dispatcher with the existing one for MMIO and removes several lines of code. This also allows to build BQL-free portio on top once we enhance the memory layer accordingly.
Seems to work fine so far but surely requires thorough review. And I would welcome early comments on the direction.
Jan
CC malc <[email protected]>Jan Kiszka (15):
adlib: replace register_ioport*
applesmc: replace register_ioport*
wdt_ib700: replace register_ioport*
i82374: replace register_ioport*
prep: replace register_ioport*
vt82c686: replace register_ioport*
Privatize register_ioport_read/write
isa: implementisa_is_ioport_assignedviamemory_region_find
memory: Introduceaddress_space_lookup_region
memory: Rework sub-page handling
memory: Allow unalignedaddress_space_rw
vmware-vga: Accept unaligned I/O accesses
ioport: Switch dispatching to memory core layer
ioport: Remove unused old dispatching services
ioport: Move IOPortRead/WriteFunc typedefs tomemory.hcputlb.c | 2 +- exec.c | 273 +++++++++++++------------------------ hw/acpi/piix4.c | 6 +- hw/audio/adlib.c | 20 ++- hw/display/vmware_vga.c | 4 + hw/dma/i82374.c | 17 ++- hw/isa/isa-bus.c | 11 ++ hw/isa/lpc_ich9.c | 8 +- hw/isa/vt82c686.c | 40 ++++-- hw/misc/applesmc.c | 48 +++++-- hw/ppc/prep.c | 23 ++- hw/watchdog/wdt_ib700.c | 12 ++- include/exec/cputlb.h | 2 - include/exec/ioport.h | 19 +--- include/exec/iorange.h | 31 ---- include/exec/memory-internal.h | 2 - include/exec/memory.h | 26 ++-- include/hw/isa/isa.h | 2 + ioport.c | 294 ++++------------------------------------ memory.c | 102 +++++--------- translate-all.c | 3 +- 21 files changed, 311 insertions(+), 634 deletions(-) delete mode 100644 include/exec/iorange.h
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Quoted 7 lines · Andreas Färber
Am 06.05.2013 16:26, schrieb Jan Kiszka:
Quoted 2 lines
This series converts the remaining users of register_ioport* to portio lists,
Why does it need to be lists? Is there anything wrong with using
isa_register_ioport()as done in Hervé's previous ioport cleanup series? (Hope you were aware of those ppc-centered discussions?)
Lists are the straight way of conversion, specifically for I/O port assignments that are scattered. On some lazy day, someone can convert them into multiple memory regions. That's beyond the scope if this series and more risky.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux