* [Qemu-devel] [PATCH 0/3] PAM emulation improvement
@ 2015-07-16 8:35 Efimov Vasily
2015-07-16 8:35 ` [Qemu-devel] [PATCH 1/3] memory: make function invalidate_and_set_dirty public Efimov Vasily
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Efimov Vasily @ 2015-07-16 8:35 UTC (permalink / raw
To: qemu-devel
Cc: Paolo Bonzini, Kirill Batuzov, Efimov Vasily, Michael S. Tsirkin
This patch series improves PAM emulation.
PAM defines 4 memory access redirection modes. In mode 1 reads are directed to
RAM and writes are directed to PCI. In mode 2 it is contrary. In mode 0 all
access is directed to PCI. In mode 3 it is directed to RAM. Modes 0 and 3 are
well emulated but modes 1 and 2 are not. The cause is: aliases are used
while more complicated logic is required.
The idea is to use ROM device like memory regions for mode 1 and 2 emulation
instead of aliases. Writes are directed to proper destination region by
specified I/O callback. Read redirection depends on type of source region.
In most cases source region is RAM (or ROM), so ram_addr of PAM region is set to
ram_addr of source region with offset. Otherwise, when source region is an I/O
region, reading is redirected to source region read callback by PAM region one.
Read source and write destination regions are updated by the memory
commit callback.
Note that we cannot use I/O region for PAM as it will violate "trying to execute
code outside RAM or ROM" assertion.
Qemu distribution includes SeaBIOS which has hacks to work around incorrect
modes 1 and 2 emulation.
This patch series is tested using modified SeaBIOS. It is forced to use mode 2
for copying its data. BIOS reads a value from memory and immediately writes it
to same address. According to PAM definition, reads are directed to PCI (i.e. to
BIOS ROM) and writes are directed to RAM.
The patch for SeaBIOS is listed below.
Both SeaBIOS versions works with new PAM but the modified one does not work with
old PAM.
======
diff --git a/src/fw/shadow.c b/src/fw/shadow.c
index 4f00006..5b0e527 100644
--- a/src/fw/shadow.c
+++ b/src/fw/shadow.c
@@ -26,32 +26,43 @@ static void
__make_bios_writable_intel(u16 bdf, u32 pam0)
{
// Make ram from 0xc0000-0xf0000 writable
- int clear = 0;
int i;
+ unsigned *mem, *mem_limit;
for (i=0; i<6; i++) {
u32 pam = pam0 + 1 + i;
int reg = pci_config_readb(bdf, pam);
- if (CONFIG_OPTIONROMS_DEPLOYED && (reg & 0x11) != 0x11) {
- // Need to copy optionroms to work around qemu implementation
- void *mem = (void*)(BUILD_ROM_START + i * 32*1024);
- memcpy((void*)BUILD_BIOS_TMP_ADDR, mem, 32*1024);
+ if ((reg & 0x11) != 0x11) {
+ mem = (unsigned *)(BUILD_ROM_START + i * 32 * 1024);
+ pci_config_writeb(bdf, pam, 0x22);
+ mem_limit = mem + 32 * 1024 / sizeof(unsigned);
+
+ while (mem < mem_limit) {
+ volatile unsigned tmp = *mem;
+ *mem = tmp;
+ mem++;
+ }
pci_config_writeb(bdf, pam, 0x33);
- memcpy(mem, (void*)BUILD_BIOS_TMP_ADDR, 32*1024);
- clear = 1;
} else {
pci_config_writeb(bdf, pam, 0x33);
}
}
- if (clear)
- memset((void*)BUILD_BIOS_TMP_ADDR, 0, 32*1024);
// Make ram from 0xf0000-0x100000 writable
int reg = pci_config_readb(bdf, pam0);
- pci_config_writeb(bdf, pam0, 0x30);
if (reg & 0x10)
// Ram already present.
return;
+ pci_config_writeb(bdf, pam0, 0x22);
+ mem = (unsigned *)BUILD_BIOS_ADDR;
+ mem_limit = mem + 32 * 1024 / sizeof(unsigned);
+ while (mem < mem_limit) {
+ volatile unsigned tmp = *mem;
+ *mem = tmp;
+ mem++;
+ }
+ pci_config_writeb(bdf, pam0, 0x33);
+
// Copy bios.
extern u8 code32flat_start[], code32flat_end[];
memcpy(code32flat_start, code32flat_start + BIOS_SRC_OFFSET
@@ -61,17 +72,6 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
static void
make_bios_writable_intel(u16 bdf, u32 pam0)
{
- int reg = pci_config_readb(bdf, pam0);
- if (!(reg & 0x10)) {
- // QEMU doesn't fully implement the piix shadow capabilities -
- // if ram isn't backing the bios segment when shadowing is
- // disabled, the code itself wont be in memory. So, run the
- // code from the high-memory flash location.
- u32 pos = (u32)__make_bios_writable_intel + BIOS_SRC_OFFSET;
- void (*func)(u16 bdf, u32 pam0) = (void*)pos;
- func(bdf, pam0);
- return;
- }
// Ram already present - just enable writes
__make_bios_writable_intel(bdf, pam0);
}
Efimov Vasily (3):
memory: make function invalidate_and_set_dirty public
memory: make function memory_access_is_direct public
PAM: make PAM emulation closer to documentation
exec.c | 14 +--
hw/pci-host/pam.c | 238 ++++++++++++++++++++++++++++++++++++-----
include/exec/memory-internal.h | 15 +++
include/hw/pci-host/pam.h | 10 +-
4 files changed, 239 insertions(+), 38 deletions(-)
--
1.9.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 1/3] memory: make function invalidate_and_set_dirty public
2015-07-16 8:35 [Qemu-devel] [PATCH 0/3] PAM emulation improvement Efimov Vasily
@ 2015-07-16 8:35 ` Efimov Vasily
2015-07-16 8:35 ` [Qemu-devel] [PATCH 2/3] memory: make function memory_access_is_direct public Efimov Vasily
2015-07-16 8:35 ` [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation Efimov Vasily
2 siblings, 0 replies; 11+ messages in thread
From: Efimov Vasily @ 2015-07-16 8:35 UTC (permalink / raw
To: qemu-devel
Cc: Paolo Bonzini, Kirill Batuzov, Efimov Vasily, Michael S. Tsirkin
Make function invalidate_and_set_dirty public. It is required by PAM emulation.
Signed-off-by: Efimov Vasily <real@ispras.ru>
---
exec.c | 2 +-
include/exec/memory-internal.h | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/exec.c b/exec.c
index 251dc79..4e37ded 100644
--- a/exec.c
+++ b/exec.c
@@ -2281,7 +2281,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
#else
-static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
+void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
hwaddr length)
{
uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index fb467ac..801da82 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -31,5 +31,8 @@ extern const MemoryRegionOps unassigned_mem_ops;
bool memory_region_access_valid(MemoryRegion *mr, hwaddr addr,
unsigned size, bool is_write);
+void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
+ hwaddr length);
+
#endif
#endif
--
1.9.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 2/3] memory: make function memory_access_is_direct public
2015-07-16 8:35 [Qemu-devel] [PATCH 0/3] PAM emulation improvement Efimov Vasily
2015-07-16 8:35 ` [Qemu-devel] [PATCH 1/3] memory: make function invalidate_and_set_dirty public Efimov Vasily
@ 2015-07-16 8:35 ` Efimov Vasily
2015-07-16 8:35 ` [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation Efimov Vasily
2 siblings, 0 replies; 11+ messages in thread
From: Efimov Vasily @ 2015-07-16 8:35 UTC (permalink / raw
To: qemu-devel
Cc: Paolo Bonzini, Kirill Batuzov, Efimov Vasily, Michael S. Tsirkin
Make function memory_access_is_direct public. It is required by PAM emulation.
Signed-off-by: Efimov Vasily <real@ispras.ru>
---
exec.c | 12 ------------
include/exec/memory-internal.h | 12 ++++++++++++
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/exec.c b/exec.c
index 4e37ded..27064b8 100644
--- a/exec.c
+++ b/exec.c
@@ -372,18 +372,6 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
return section;
}
-static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
-{
- if (memory_region_is_ram(mr)) {
- return !(is_write && mr->readonly);
- }
- if (memory_region_is_romd(mr)) {
- return !is_write;
- }
-
- return false;
-}
-
/* Called from RCU critical section */
MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
hwaddr *xlat, hwaddr *plen,
diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 801da82..89975b6 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -34,5 +34,17 @@ bool memory_region_access_valid(MemoryRegion *mr, hwaddr addr,
void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
hwaddr length);
+static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
+{
+ if (memory_region_is_ram(mr)) {
+ return !(is_write && mr->readonly);
+ }
+ if (memory_region_is_romd(mr)) {
+ return !is_write;
+ }
+
+ return false;
+}
+
#endif
#endif
--
1.9.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation
2015-07-16 8:35 [Qemu-devel] [PATCH 0/3] PAM emulation improvement Efimov Vasily
2015-07-16 8:35 ` [Qemu-devel] [PATCH 1/3] memory: make function invalidate_and_set_dirty public Efimov Vasily
2015-07-16 8:35 ` [Qemu-devel] [PATCH 2/3] memory: make function memory_access_is_direct public Efimov Vasily
@ 2015-07-16 8:35 ` Efimov Vasily
2015-07-16 9:05 ` Paolo Bonzini
2 siblings, 1 reply; 11+ messages in thread
From: Efimov Vasily @ 2015-07-16 8:35 UTC (permalink / raw
To: qemu-devel
Cc: Paolo Bonzini, Kirill Batuzov, Efimov Vasily, Michael S. Tsirkin
This patch improves PAM emulation.
PAM defines 4 memory access redirection modes. In mode 1 reads are directed to
RAM and writes are directed to PCI. In mode 2 it is contrary. In mode 0 all
access is directed to PCI. In mode 3 it is directed to RAM. Modes 0 and 3 are
well emulated but modes 1 and 2 are not. The cause is: aliases are used
while more complicated logic is required.
The idea is to use ROM device like memory regions for mode 1 and 2 emulation
instead of aliases. Writes are directed to proper destination region by
specified I/O callback. Read redirection depends on type of source region.
In most cases source region is RAM (or ROM), so ram_addr of PAM region is set to
ram_addr of source region with offset. Otherwise, when source region is an I/O
region, reading is redirected to source region read callback by PAM region one.
Read source and write destination regions are updated by the memory
commit callback.
Note that we cannot use I/O region for PAM as it will violate "trying to execute
code outside RAM or ROM" assertion.
Signed-off-by: Efimov Vasily <real@ispras.ru>
---
hw/pci-host/pam.c | 238 +++++++++++++++++++++++++++++++++++++++++-----
include/hw/pci-host/pam.h | 10 +-
2 files changed, 223 insertions(+), 25 deletions(-)
diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
index 17d826c..9729b2b 100644
--- a/hw/pci-host/pam.c
+++ b/hw/pci-host/pam.c
@@ -27,43 +27,233 @@
* THE SOFTWARE.
*/
-#include "qom/object.h"
-#include "sysemu/sysemu.h"
#include "hw/pci-host/pam.h"
+#include "exec/address-spaces.h"
+#include "exec/memory-internal.h"
+#include "qemu/bswap.h"
+
+static void pam_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+ PAMMemoryRegion *pam = (PAMMemoryRegion *) opaque;
+ void *ptr;
+
+ /* Destination region can be both RAM and IO. */
+ if (!memory_access_is_direct(pam->mr_write_to, true)) {
+ memory_region_dispatch_write(pam->mr_write_to,
+ addr + pam->write_offset, data, size,
+ MEMTXATTRS_UNSPECIFIED);
+ } else {
+ ptr = memory_region_get_ram_ptr(pam->mr_write_to) + addr
+ + pam->write_offset;
+
+ switch (size) {
+ case 1:
+ stb_p(ptr, data);
+ break;
+ case 2:
+ stw_he_p(ptr, data);
+ break;
+ case 4:
+ stl_he_p(ptr, data);
+ break;
+ case 8:
+ stq_he_p(ptr, data);
+ break;
+ default:
+ abort();
+ }
+
+ invalidate_and_set_dirty(pam->mr_write_to, addr + pam->pam_offset,
+ size);
+ }
+}
+
+static uint64_t pam_read(void *opaque, hwaddr addr, unsigned size)
+{
+ PAMMemoryRegion *pam = (PAMMemoryRegion *) opaque;
+ uint64_t ret = 0;
+
+ /* Source region can be IO only. */
+ memory_region_dispatch_read(pam->mr_read_from, pam->read_offset + addr,
+ &ret, size, MEMTXATTRS_UNSPECIFIED);
+
+ return ret;
+}
+
+static MemoryRegionOps pam_ops = {
+ .write = pam_write,
+ .read = pam_read,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void pam_set_current(PAMMemoryRegion *pam, unsigned new_current)
+{
+ assert(new_current <= 3);
+
+ if (new_current != pam->current) {
+#ifdef DEBUG_PAM
+ printf("PAM 0x"TARGET_FMT_plx": %d <=> %s\n", pam->pam_offset,
+ new_current, pam->region[new_current].name);
+#endif
+ memory_region_set_enabled(&pam->region[pam->current], false);
+ pam->current = new_current;
+ memory_region_set_enabled(&pam->region[new_current], true);
+ }
+}
+
+static void pam_leaf_mr_lookup(MemoryRegion *mr, hwaddr offset,
+ MemoryRegion **leaf, hwaddr *offset_within_leaf)
+{
+ MemoryRegion *other;
+ if (mr->alias) {
+ pam_leaf_mr_lookup(mr->alias, offset + mr->alias_offset, leaf,
+ offset_within_leaf);
+ } else {
+ if (QTAILQ_EMPTY(&mr->subregions)
+ && int128_lt(int128_make64(offset), mr->size)) {
+ *leaf = mr;
+ *offset_within_leaf = offset;
+ } else {
+ QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
+ if (!other->enabled) {
+ continue;
+ }
+ if (other->addr <= offset && int128_lt(int128_make64(offset),
+ int128_add(int128_make64(other->addr), other->size))) {
+ pam_leaf_mr_lookup(other, offset - other->addr, leaf,
+ offset_within_leaf);
+ if (*leaf) {
+ return;
+ }
+ }
+ }
+ *leaf = NULL;
+ }
+ }
+}
+
+static bool mr_has_mr(MemoryRegion *container, MemoryRegion *mr,
+ hwaddr *offset)
+{
+ hwaddr tmp_offset;
+ MemoryRegion *other;
+
+ if (container == mr) {
+ *offset = 0;
+ return true;
+ }
+ if (container->alias) {
+ if (mr_has_mr(container->alias, mr, &tmp_offset)) {
+ *offset = tmp_offset - container->alias_offset;
+ return true;
+ } else {
+ return false;
+ }
+ }
+ QTAILQ_FOREACH(other, &container->subregions, subregions_link) {
+ if (mr_has_mr(other, mr, &tmp_offset)) {
+ *offset = tmp_offset + other->addr;
+ return true;
+ }
+ }
+ return false;
+}
+
+static void pam_mem_commit(MemoryListener *listener)
+{
+ PAMMemoryRegion *pam = container_of(listener, PAMMemoryRegion, listener);
+ MemoryRegion *cur, *read_src, *write_dst;
+ hwaddr offset_within_leaf;
+
+ if (pam->current == 1) {
+ /* Read from RAM and write to PCI */
+ /* Note pam->region[3].alias points to RAM by creation. */
+ read_src = pam->region[3].alias;
+ pam->read_offset = pam->pam_offset;
+
+ pam_leaf_mr_lookup(&pam->region[0], 0, &write_dst, &offset_within_leaf);
+
+ assert(write_dst);
+
+ pam->write_offset = offset_within_leaf;
+ } else if (pam->current == 2) {
+ /* Read from PCI and write to RAM */
+ pam_leaf_mr_lookup(&pam->region[0], 0, &read_src, &offset_within_leaf);
+
+ assert(read_src);
+
+ pam->read_offset = offset_within_leaf;
+
+ write_dst = pam->region[3].alias;
+ pam->write_offset = pam->pam_offset;
+ } else {
+ return;
+ }
+
+ cur = &pam->region[pam->current];
+
+ if (memory_region_is_ram(read_src)) {
+ /* Current region parent is system_memory by creation. */
+ assert(mr_has_mr(cur->container, read_src, &offset_within_leaf));
+ cur->ram_addr = read_src->ram_addr + pam->pam_offset
+ - offset_within_leaf;
+ cur->rom_device = true;
+ cur->romd_mode = true;
+ } else {
+ cur->ram_addr = ~(ram_addr_t)0;
+ cur->rom_device = false;
+ cur->romd_mode = false;
+ }
+
+ pam->mr_read_from = read_src;
+ pam->mr_write_to = write_dst;
+}
void init_pam(DeviceState *dev, MemoryRegion *ram_memory,
MemoryRegion *system_memory, MemoryRegion *pci_address_space,
- PAMMemoryRegion *mem, uint32_t start, uint32_t size)
+ PAMMemoryRegion *pam, uint32_t start, uint32_t size)
{
int i;
- /* RAM */
- memory_region_init_alias(&mem->alias[3], OBJECT(dev), "pam-ram", ram_memory,
- start, size);
- /* ROM (XXX: not quite correct) */
- memory_region_init_alias(&mem->alias[1], OBJECT(dev), "pam-rom", ram_memory,
- start, size);
- memory_region_set_readonly(&mem->alias[1], true);
-
- /* XXX: should distinguish read/write cases */
- memory_region_init_alias(&mem->alias[0], OBJECT(dev), "pam-pci", pci_address_space,
- start, size);
- memory_region_init_alias(&mem->alias[2], OBJECT(dev), "pam-pci", ram_memory,
- start, size);
-
- for (i = 0; i < 4; ++i) {
- memory_region_set_enabled(&mem->alias[i], false);
+ /* All access to PCI */
+ memory_region_init_alias(&pam->region[0], OBJECT(dev), "pam-pci",
+ pci_address_space, start, size);
+
+ /* Read from RAM and write to PCI */
+ memory_region_init_io(&pam->region[1], OBJECT(dev), &pam_ops, pam,
+ "pam-r-ram-w-pci", size);
+
+ /* Read from PCI and write to RAM */
+ memory_region_init_io(&pam->region[2], OBJECT(dev), &pam_ops, pam,
+ "pam-r-pci-w-ram", size);
+
+ /* All access to RAM */
+ memory_region_init_alias(&pam->region[3], OBJECT(dev), "pam-ram",
+ ram_memory, start, size);
+
+ memory_region_add_subregion_overlap(system_memory, start,
+ &pam->region[0], 1);
+ memory_region_set_enabled(&pam->region[0], true);
+ for (i = 1; i < 4; i++) {
+ memory_region_set_enabled(&pam->region[i], false);
memory_region_add_subregion_overlap(system_memory, start,
- &mem->alias[i], 1);
+ &pam->region[i], 1);
}
- mem->current = 0;
+
+ pam->current = 0;
+ pam->pam_offset = start;
+
+ pam->listener = (MemoryListener) {
+ .commit = pam_mem_commit,
+ };
+
+ memory_listener_register(&pam->listener, &address_space_memory);
}
void pam_update(PAMMemoryRegion *pam, int idx, uint8_t val)
{
assert(0 <= idx && idx <= 12);
- memory_region_set_enabled(&pam->alias[pam->current], false);
- pam->current = (val >> ((!(idx & 1)) * 4)) & PAM_ATTR_MASK;
- memory_region_set_enabled(&pam->alias[pam->current], true);
+ pam_set_current(pam, (val >> ((!(idx & 1)) * 4)) & PAM_ATTR_MASK);
}
diff --git a/include/hw/pci-host/pam.h b/include/hw/pci-host/pam.h
index 6116c63..b918029 100644
--- a/include/hw/pci-host/pam.h
+++ b/include/hw/pci-host/pam.h
@@ -53,6 +53,8 @@
#include "qemu-common.h"
#include "exec/memory.h"
+/* #define DEBUG_PAM */
+
#define SMRAM_C_BASE 0xa0000
#define SMRAM_C_END 0xc0000
#define SMRAM_C_SIZE 0x20000
@@ -82,8 +84,14 @@
#define SMRAM_C_BASE_SEG ((uint8_t)0x2) /* hardwired to b010 */
typedef struct PAMMemoryRegion {
- MemoryRegion alias[4]; /* index = PAM value */
+ MemoryRegion region[4]; /* index = PAM value */
unsigned current;
+ hwaddr pam_offset;
+ MemoryRegion *mr_write_to;
+ hwaddr write_offset;
+ MemoryRegion *mr_read_from;
+ hwaddr read_offset;
+ MemoryListener listener;
} PAMMemoryRegion;
void init_pam(DeviceState *dev, MemoryRegion *ram, MemoryRegion *system,
--
1.9.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation
2015-07-16 8:35 ` [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation Efimov Vasily
@ 2015-07-16 9:05 ` Paolo Bonzini
2015-07-16 10:51 ` Ефимов Василий
0 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2015-07-16 9:05 UTC (permalink / raw
To: Efimov Vasily, qemu-devel; +Cc: Michael S. Tsirkin, Kirill Batuzov
On 16/07/2015 10:35, Efimov Vasily wrote:
> This patch improves PAM emulation.
>
> PAM defines 4 memory access redirection modes. In mode 1 reads are directed to
> RAM and writes are directed to PCI. In mode 2 it is contrary. In mode 0 all
> access is directed to PCI. In mode 3 it is directed to RAM. Modes 0 and 3 are
> well emulated but modes 1 and 2 are not. The cause is: aliases are used
> while more complicated logic is required.
>
> The idea is to use ROM device like memory regions for mode 1 and 2 emulation
> instead of aliases. Writes are directed to proper destination region by
> specified I/O callback. Read redirection depends on type of source region.
> In most cases source region is RAM (or ROM), so ram_addr of PAM region is set to
> ram_addr of source region with offset. Otherwise, when source region is an I/O
> region, reading is redirected to source region read callback by PAM region one.
>
> Read source and write destination regions are updated by the memory
> commit callback.
>
> Note that we cannot use I/O region for PAM as it will violate "trying to execute
> code outside RAM or ROM" assertion.
>
> Signed-off-by: Efimov Vasily <real@ispras.ru>
> ---
> hw/pci-host/pam.c | 238 +++++++++++++++++++++++++++++++++++++++++-----
> include/hw/pci-host/pam.h | 10 +-
> 2 files changed, 223 insertions(+), 25 deletions(-)
>
> diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
> index 17d826c..9729b2b 100644
> --- a/hw/pci-host/pam.c
> +++ b/hw/pci-host/pam.c
> @@ -27,43 +27,233 @@
> * THE SOFTWARE.
> */
>
> -#include "qom/object.h"
> -#include "sysemu/sysemu.h"
> #include "hw/pci-host/pam.h"
> +#include "exec/address-spaces.h"
> +#include "exec/memory-internal.h"
> +#include "qemu/bswap.h"
> +
> +static void pam_write(void *opaque, hwaddr addr, uint64_t data,
> + unsigned size)
> +{
> + PAMMemoryRegion *pam = (PAMMemoryRegion *) opaque;
> + void *ptr;
> +
> + /* Destination region can be both RAM and IO. */
> + if (!memory_access_is_direct(pam->mr_write_to, true)) {
> + memory_region_dispatch_write(pam->mr_write_to,
> + addr + pam->write_offset, data, size,
> + MEMTXATTRS_UNSPECIFIED);
> + } else {
> + ptr = memory_region_get_ram_ptr(pam->mr_write_to) + addr
> + + pam->write_offset;
> +
> + switch (size) {
> + case 1:
> + stb_p(ptr, data);
> + break;
> + case 2:
> + stw_he_p(ptr, data);
> + break;
> + case 4:
> + stl_he_p(ptr, data);
> + break;
> + case 8:
> + stq_he_p(ptr, data);
> + break;
> + default:
> + abort();
> + }
> +
> + invalidate_and_set_dirty(pam->mr_write_to, addr + pam->pam_offset,
> + size);
> + }
> +}
> +
The idea is very good, but the implementation relies on copying a lot of
code from exec.c.
Could you use an IOMMU memory region instead? Then a single region can
be used to implement all four modes, and you don't hit the "trying to
execute code outside RAM or RAM".
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation
2015-07-16 9:05 ` Paolo Bonzini
@ 2015-07-16 10:51 ` Ефимов Василий
2015-07-16 11:10 ` Paolo Bonzini
0 siblings, 1 reply; 11+ messages in thread
From: Ефимов Василий @ 2015-07-16 10:51 UTC (permalink / raw
To: Paolo Bonzini, qemu-devel; +Cc: Michael S. Tsirkin, Kirill Batuzov
16.07.2015 12:05, Paolo Bonzini пишет:
>
>
> On 16/07/2015 10:35, Efimov Vasily wrote:
>> This patch improves PAM emulation.
>>
>> PAM defines 4 memory access redirection modes. In mode 1 reads are directed to
>> RAM and writes are directed to PCI. In mode 2 it is contrary. In mode 0 all
>> access is directed to PCI. In mode 3 it is directed to RAM. Modes 0 and 3 are
>> well emulated but modes 1 and 2 are not. The cause is: aliases are used
>> while more complicated logic is required.
>>
>> The idea is to use ROM device like memory regions for mode 1 and 2 emulation
>> instead of aliases. Writes are directed to proper destination region by
>> specified I/O callback. Read redirection depends on type of source region.
>> In most cases source region is RAM (or ROM), so ram_addr of PAM region is set to
>> ram_addr of source region with offset. Otherwise, when source region is an I/O
>> region, reading is redirected to source region read callback by PAM region one.
>>
>> Read source and write destination regions are updated by the memory
>> commit callback.
>>
>> Note that we cannot use I/O region for PAM as it will violate "trying to execute
>> code outside RAM or ROM" assertion.
>>
>> Signed-off-by: Efimov Vasily <real@ispras.ru>
>> ---
>> hw/pci-host/pam.c | 238 +++++++++++++++++++++++++++++++++++++++++-----
>> include/hw/pci-host/pam.h | 10 +-
>> 2 files changed, 223 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
>> index 17d826c..9729b2b 100644
>> --- a/hw/pci-host/pam.c
>> +++ b/hw/pci-host/pam.c
>> @@ -27,43 +27,233 @@
>> * THE SOFTWARE.
>> */
>>
>> -#include "qom/object.h"
>> -#include "sysemu/sysemu.h"
>> #include "hw/pci-host/pam.h"
>> +#include "exec/address-spaces.h"
>> +#include "exec/memory-internal.h"
>> +#include "qemu/bswap.h"
>> +
>> +static void pam_write(void *opaque, hwaddr addr, uint64_t data,
>> + unsigned size)
>> +{
>> + PAMMemoryRegion *pam = (PAMMemoryRegion *) opaque;
>> + void *ptr;
>> +
>> + /* Destination region can be both RAM and IO. */
>> + if (!memory_access_is_direct(pam->mr_write_to, true)) {
>> + memory_region_dispatch_write(pam->mr_write_to,
>> + addr + pam->write_offset, data, size,
>> + MEMTXATTRS_UNSPECIFIED);
>> + } else {
>> + ptr = memory_region_get_ram_ptr(pam->mr_write_to) + addr
>> + + pam->write_offset;
>> +
>> + switch (size) {
>> + case 1:
>> + stb_p(ptr, data);
>> + break;
>> + case 2:
>> + stw_he_p(ptr, data);
>> + break;
>> + case 4:
>> + stl_he_p(ptr, data);
>> + break;
>> + case 8:
>> + stq_he_p(ptr, data);
>> + break;
>> + default:
>> + abort();
>> + }
>> +
>> + invalidate_and_set_dirty(pam->mr_write_to, addr + pam->pam_offset,
>> + size);
>> + }
>> +}
>> +
>
> The idea is very good, but the implementation relies on copying a lot of
> code from exec.c.
If it is about pam_write then, for instance, I could suggest to outline
corresponding part of address_space_rw to dedicated public function.
The solution will require endianness conversion because of the
part works with uint8_t buffer but not with uint64_t values.
The rest of code looks up destination or source region or child region
offset in memory sub-tree which root is PCI or RAM region provided on
PAM creation. We cannon use common address_space_translate because it
searches from address space root and will return current PAM region.
To summarize, I suggest to move the code to exec.c. It is generic
enough.
>
> Could you use an IOMMU memory region instead? Then a single region can
> be used to implement all four modes, and you don't hit the "trying to
> execute code outside RAM or RAM".
Did you mean MemoryRegion.iommu_ops ? The feature does not allow to
change destination memory region. Also I has no find its using during
write access from CPU. And there is:
exec.c: address_space_translate_for_iotlb:
assert(!section->mr->iommu_ops);
>
> Paolo
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation
2015-07-16 10:51 ` Ефимов Василий
@ 2015-07-16 11:10 ` Paolo Bonzini
2015-07-16 14:41 ` Ефимов Василий
0 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2015-07-16 11:10 UTC (permalink / raw
To: Ефимов Василий,
qemu-devel
Cc: Kirill Batuzov, Michael S. Tsirkin
On 16/07/2015 12:51, Ефимов Василий wrote:
> The rest of code looks up destination or source region or child region
> offset in memory sub-tree which root is PCI or RAM region provided on
> PAM creation. We cannon use common address_space_translate because it
> searches from address space root and will return current PAM region.
> To summarize, I suggest to move the code to exec.c. It is generic
> enough.
All these mechanism are extremely low level. They are encapsulated
within exec.c, and copying code to pam.c is not a good idea because you
already have all the AddressSpaces and RAM MemoryRegions you need.
>> Could you use an IOMMU memory region instead? Then a single region can
>> be used to implement all four modes, and you don't hit the "trying to
>> execute code outside RAM or RAM".
> Did you mean MemoryRegion.iommu_ops ? The feature does not allow to
> change destination memory region.
It does. You're right about this:
> exec.c: address_space_translate_for_iotlb:
> assert(!section->mr->iommu_ops);
... but an IOMMU region is not needed, and I think you can do everything
without touching exec.c at all.
+ /* Read from RAM and write to PCI */
+ memory_region_init_io(&pam->region[1], OBJECT(dev), &pam_ops, pam,
+ "pam-r-ram-w-pci", size);
This can be done with memory_region_set_readonly on the RAM region. You
need to set mr->ops in order to point to pam_ops; for a first proof of
concept you can just set the field directly.
Writes to the PCI memory space can use the PCI address space, with
address_space_st*.
+ /* Read from PCI and write to RAM */
+ memory_region_init_io(&pam->region[2], OBJECT(dev), &pam_ops, pam,
+ "pam-r-pci-w-ram", size);
Here you cannot run code from ROM, so it can be a pure MMIO region.
Reads can use address_space_ld*, while writes can use
memory_region_get_ram_ptr.
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation
2015-07-16 11:10 ` Paolo Bonzini
@ 2015-07-16 14:41 ` Ефимов Василий
2015-07-16 17:52 ` Paolo Bonzini
0 siblings, 1 reply; 11+ messages in thread
From: Ефимов Василий @ 2015-07-16 14:41 UTC (permalink / raw
To: Paolo Bonzini, qemu-devel; +Cc: Kirill Batuzov, Michael S. Tsirkin
16.07.2015 14:10, Paolo Bonzini wrote:
>
>
> On 16/07/2015 12:51, Ефимов Василий wrote:
>> The rest of code looks up destination or source region or child region
>> offset in memory sub-tree which root is PCI or RAM region provided on
>> PAM creation. We cannon use common address_space_translate because it
>> searches from address space root and will return current PAM region.
>> To summarize, I suggest to move the code to exec.c. It is generic
>> enough.
>
> All these mechanism are extremely low level. They are encapsulated
> within exec.c, and copying code to pam.c is not a good idea because you
> already have all the AddressSpaces and RAM MemoryRegions you need.
The core problem is there is no region type which can redirects access
depending on whether it is read or write. The access type driven alias
could be perfect. But it is quite difficult to invent such type of
alias (or to generalize existing). The main problem is rendering memory
tree to FlatView.
>
>>> Could you use an IOMMU memory region instead? Then a single region can
>>> be used to implement all four modes, and you don't hit the "trying to
>>> execute code outside RAM or RAM".
>> Did you mean MemoryRegion.iommu_ops ? The feature does not allow to
>> change destination memory region.
>
> It does. You're right about this:
>
>> exec.c: address_space_translate_for_iotlb:
>> assert(!section->mr->iommu_ops);
>
> ... but an IOMMU region is not needed, and I think you can do everything
> without touching exec.c at all.
>
> + /* Read from RAM and write to PCI */
> + memory_region_init_io(&pam->region[1], OBJECT(dev), &pam_ops, pam,
> + "pam-r-ram-w-pci", size);
>
> This can be done with memory_region_set_readonly on the RAM region. You
> need to set mr->ops in order to point to pam_ops; for a first proof of
> concept you can just set the field directly.
The idea is to read directly from system RAM region and to write
to PCI using I/O (because I do not see another way to emulate
access type driven redirection with existent API). If we create RAM
and make it read only then new useless RAM block will be created.
It is useless because of ram_addr of new region will be set to
one within system RAM block. Hence, cleaner way is to create I/O region.
>
> Writes to the PCI memory space can use the PCI address space, with
> address_space_st*.
There is no PCI AddressSpace (only MemoryRegion). But
address_space_st* requires AddressSpace as argument.
>
> + /* Read from PCI and write to RAM */
> + memory_region_init_io(&pam->region[2], OBJECT(dev), &pam_ops, pam,
> + "pam-r-pci-w-ram", size);
>
> Here you cannot run code from ROM, so it can be a pure MMIO region.
> Reads can use address_space_ld*, while writes can use
> memory_region_get_ram_ptr.
Even in this mode it is possible for code to be executed from ROM. This
can happen when particular PCI address is within ROM device connected
to PCI bus.
I do not have examples where this happens in mode 2, but in mode 0
(where reads are also directed to PCI) this happens at startup time
when BIOS is executed from ROM device on PCI. The path in memory tree
is "system - pam-pci - pci - isa-bios - pc.bios" where pc.bios is ROM
and pam-pci is alias.
If this happens when PAM is in mode 2 then path should become
"system - pam-r-pci-w-ram" where pam-r-pci-w-ram is ROM which ram_addr
points to RAM block of pc.bios. Write handler of pam-r-pci-w-ram
performs access to RAM block of pc.ram.
Implemented mechanism performs search for leaf region. And it supports
cases when leaf is I/O too. In this case pam-r-pci-w-ram (and
pam-r-ram-w-pci) becomes clear I/O and both its handlers
redirects access to corresponding region.
>
> Paolo
>
Vasily
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation
2015-07-16 14:41 ` Ефимов Василий
@ 2015-07-16 17:52 ` Paolo Bonzini
2015-07-17 9:50 ` Ефимов Василий
0 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2015-07-16 17:52 UTC (permalink / raw
To: Ефимов Василий,
qemu-devel
Cc: Kirill Batuzov, Michael S. Tsirkin
On 16/07/2015 16:41, Ефимов Василий wrote:
> The main problem is rendering memory tree to FlatView.
I don't believe it's necessary to render a memory tree to the FlatView.
You can use existing AddressSpaces.
>> + /* Read from RAM and write to PCI */
>> + memory_region_init_io(&pam->region[1], OBJECT(dev), &pam_ops, pam,
>> + "pam-r-ram-w-pci", size);
>>
>> This can be done with memory_region_set_readonly on the RAM region. You
>> need to set mr->ops in order to point to pam_ops; for a first proof of
>> concept you can just set the field directly.
> The idea is to read directly from system RAM region and to write
> to PCI using I/O (because I do not see another way to emulate
> access type driven redirection with existent API). If we create RAM
> and make it read only then new useless RAM block will be created.
Don't create RAM; modify the existing one.
> It is useless because of ram_addr of new region will be set to
> one within system RAM block. Hence, cleaner way is to create I/O region.
You can use the existing RAM region and modify its properties (i.e.
toggle mr->readonly) after setting special mr->ops. The special mr->ops
will be used for writes when mr->readonly = true.
>> Writes to the PCI memory space can use the PCI address space, with
>> address_space_st*.
> There is no PCI AddressSpace (only MemoryRegion). But
> address_space_st* requires AddressSpace as argument.
Then create one with address_space_init.
However, can the guest see the difference between "real" mode 1 and the
"fake" mode 1 that QEMU implements? Perhaps mode 1 can be left as is.
>> + /* Read from PCI and write to RAM */
>> + memory_region_init_io(&pam->region[2], OBJECT(dev), &pam_ops, pam,
>> + "pam-r-pci-w-ram", size);
>>
>> Here you cannot run code from ROM, so it can be a pure MMIO region.
>> Reads can use address_space_ld*, while writes can use
>> memory_region_get_ram_ptr.
>
> Even in this mode it is possible for code to be executed from ROM. This
> can happen when particular PCI address is within ROM device connected
> to PCI bus.
If it's just for pc.rom and isa-bios, introduce a new function
pam_create_pci_region that creates pc.rom with
memory_region_init_rom_device. The mr->ops can write to RAM (mode 2) or
discard the write (mode 0).
They you can make pc.rom 256K instead of 128K, and instead of an alias,
you can manually copy the last 128K of the BIOS into the last 128K of
pc.rom.
Some adjustment will be necessary in order to support migration (perhaps
creating two 128K regions pc.rom and pc.rom.mirror), but for a proof of
concept the above should be enough.
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation
2015-07-16 17:52 ` Paolo Bonzini
@ 2015-07-17 9:50 ` Ефимов Василий
2015-07-17 10:10 ` Paolo Bonzini
0 siblings, 1 reply; 11+ messages in thread
From: Ефимов Василий @ 2015-07-17 9:50 UTC (permalink / raw
To: Paolo Bonzini, qemu-devel; +Cc: Kirill Batuzov, Michael S. Tsirkin
16.07.2015 20:52, Paolo Bonzini пишет:
>
>
> On 16/07/2015 16:41, Ефимов Василий wrote:
>> The main problem is rendering memory tree to FlatView.
>
> I don't believe it's necessary to render a memory tree to the FlatView.
> You can use existing AddressSpaces.
>
>>> + /* Read from RAM and write to PCI */
>>> + memory_region_init_io(&pam->region[1], OBJECT(dev), &pam_ops, pam,
>>> + "pam-r-ram-w-pci", size);
>>>
>>> This can be done with memory_region_set_readonly on the RAM region. You
>>> need to set mr->ops in order to point to pam_ops; for a first proof of
>>> concept you can just set the field directly.
>> The idea is to read directly from system RAM region and to write
>> to PCI using I/O (because I do not see another way to emulate
>> access type driven redirection with existent API). If we create RAM
>> and make it read only then new useless RAM block will be created.
>
> Don't create RAM; modify the existing one.
>
>> It is useless because of ram_addr of new region will be set to
>> one within system RAM block. Hence, cleaner way is to create I/O region.
>
> You can use the existing RAM region and modify its properties (i.e.
> toggle mr->readonly) after setting special mr->ops. The special mr->ops
> will be used for writes when mr->readonly = true.
The existing RAMs are ether whole pc.ram or pc.rom and pc.bios beyond
PCI. So, I think we have not invade them. Also, we only need the small
subrange to be read only. Moreover, if -pflash is used then there is
ROM device instead of pc.bios with its own mr->ops.
You have suggested to create AddressSpace for PCI below. It is flexible
solution which is transparent for existing regions. I suggests to create
AddressSpace for RAM subtree too. I think, both AS have to be PAM
implementation private for complete transparency for another QEMU parts.
I will try it in next patch version.
>
>>> Writes to the PCI memory space can use the PCI address space, with
>>> address_space_st*.
>> There is no PCI AddressSpace (only MemoryRegion). But
>> address_space_st* requires AddressSpace as argument.
>
> Then create one with address_space_init.
>
> However, can the guest see the difference between "real" mode 1 and the
> "fake" mode 1 that QEMU implements? Perhaps mode 1 can be left as is.
Yes, guest can.
If -pfalsh is used then BIOS becomes a programmable ROM device. Then
guest can switch to mode 1 and copy data by reading and writing it
at same location. After that guest can switch to mode 0 (or 3) and
copy data to some another place. Then it switches to mode 3 (or 0) and
compares the data.
I just check it with SeaBIOS and original PAM adding code listed below
to fw/shadow.c: __make_bios_writable_intel.
dprintf(1, "PAM mode 1 test begin\n");
unsigned *m = (unsigned *) BUILD_ROM_START;
pci_config_writeb(bdf, pam0 + 1, 0x33);
*m = 0xdeafbeef;
pci_config_writeb(bdf, pam0 + 1, 0x11);
volatile unsigned t = *m;
*m = t;
pci_config_writeb(bdf, pam0 + 1, 0x33);
t = *m;
pci_config_writeb(bdf, pam0 + 1, 0x00);
unsigned t2 = *m;
dprintf(1, "t = 0x%x, t2 = 0x%x\n", t, t2);
dprintf(1, "PAM mode 1 test end\n");
The output is:
PAM mode 1 test begin
t = 0xdeafbeef, t2 = 0x0
PAM mode 1 test end
With new PAM the output is:
PAM mode 1 test begin
t = 0xdeafbeef, t2 = 0xdeafbeef
PAM mode 1 test end
Note BUILD_ROM_START is 0xc0000 which is pc.rom with write permission
according to info mtree output. The -pflash is not needed.
>
>>> + /* Read from PCI and write to RAM */
>>> + memory_region_init_io(&pam->region[2], OBJECT(dev), &pam_ops, pam,
>>> + "pam-r-pci-w-ram", size);
>>>
>>> Here you cannot run code from ROM, so it can be a pure MMIO region.
>>> Reads can use address_space_ld*, while writes can use
>>> memory_region_get_ram_ptr.
>>
>> Even in this mode it is possible for code to be executed from ROM. This
>> can happen when particular PCI address is within ROM device connected
>> to PCI bus.
>
> If it's just for pc.rom and isa-bios, introduce a new function
> pam_create_pci_region that creates pc.rom with
> memory_region_init_rom_device. The mr->ops can write to RAM (mode 2) or
> discard the write (mode 0).
>
> They you can make pc.rom 256K instead of 128K, and instead of an alias,
> you can manually copy the last 128K of the BIOS into the last 128K of
> pc.rom.
>
> Some adjustment will be necessary in order to support migration (perhaps
> creating two 128K regions pc.rom and pc.rom.mirror), but for a proof of
> concept the above should be enough.
I will try solution based on address spaces as stated above. It is
cleaner and more generic.
>
> Paolo
>
Vasily
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation
2015-07-17 9:50 ` Ефимов Василий
@ 2015-07-17 10:10 ` Paolo Bonzini
0 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2015-07-17 10:10 UTC (permalink / raw
To: Ефимов Василий,
qemu-devel
Cc: Michael S. Tsirkin, Kirill Batuzov
On 17/07/2015 11:50, Ефимов Василий wrote:
> I will try solution based on address spaces as stated above.
If it works, that would be great.
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-07-17 10:10 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-16 8:35 [Qemu-devel] [PATCH 0/3] PAM emulation improvement Efimov Vasily
2015-07-16 8:35 ` [Qemu-devel] [PATCH 1/3] memory: make function invalidate_and_set_dirty public Efimov Vasily
2015-07-16 8:35 ` [Qemu-devel] [PATCH 2/3] memory: make function memory_access_is_direct public Efimov Vasily
2015-07-16 8:35 ` [Qemu-devel] [PATCH 3/3] PAM: make PAM emulation closer to documentation Efimov Vasily
2015-07-16 9:05 ` Paolo Bonzini
2015-07-16 10:51 ` Ефимов Василий
2015-07-16 11:10 ` Paolo Bonzini
2015-07-16 14:41 ` Ефимов Василий
2015-07-16 17:52 ` Paolo Bonzini
2015-07-17 9:50 ` Ефимов Василий
2015-07-17 10:10 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.