memory: Fix old portio word accesses

From: Jan Kiszka<[email protected]>

As we register old portio regions via ioport_register, we are also responsible for providing the word access wrapper.

Signed-off-by Jan Kiszka<[email protected]>

---

Oops, was lacking a shift for word reads.

  memory.c |   10 ++++++++++
  1 files changed, 10 insertions(+), 0 deletions(-)

memory.c +0 -0

index b3ee232..aef4702 100644

Quoted 23 lines · Jan Kiszka

From: Jan Kiszka<[email protected]>

As we register old portio regions via ioport_register, we are also responsible for providing the word access wrapper.

Signed-off-by Jan Kiszka<[email protected]>

---

Oops, was lacking a shift for word reads.

  memory.c |   10 ++++++++++
  1 files changed, 10 insertions(+), 0 deletions(-)

memory.c +1 -0

index b3ee232..aef4702 100644
@@ -397,6 +397,11 @@ static void memory_region_iorange_read(IORange
 *iorange,
          *data = ((uint64_t)1<<  (width * 8)) - 1;
          if (mrp) {
              *data = mrp->read(mr->opaque, offset + mr->offset);
+        } else if (width == 2) {

memory.c static void memory_region_iorange_read(IORange *iorange, · line 405

Quoted 6 lines

memory.c +4 -0

+            mrp = find_portio(mr, offset, 1, false);
+            assert(mrp);
+            *data = mrp->read(mr->opaque, offset + mr->offset) |
+                    (mrp->read(mr->opaque, offset + mr->offset +
 1)<<  8);
          }

What about width 4?

Why not use access_with_adjusted_size()?

Quoted 31 lines · Avi Kivity

Quoted 29 lines · Jan Kiszka

From: Jan Kiszka<[email protected]>

As we register old portio regions via ioport_register, we are also responsible for providing the word access wrapper.

Signed-off-by Jan Kiszka<[email protected]>

---

Oops, was lacking a shift for word reads.

   memory.c |   10 ++++++++++
   1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index b3ee232..aef4702 100644
--- a/memory.c
+++ b/memory.c
@@ -397,6 +397,11 @@ static void memory_region_iorange_read(IORange *iorange,

           *data = ((uint64_t)1<<   (width * 8)) - 1;
           if (mrp) {
               *data = mrp->read(mr->opaque, offset + mr->offset);
 +        } else if (width == 2) {
 +            mrp = find_portio(mr, offset, 1, false);
 +            assert(mrp);
 +            *data = mrp->read(mr->opaque, offset + mr->offset) |
 +                    (mrp->read(mr->opaque, offset + mr->offset +
 1)<<   8);
           }

Quoted 1 line

What about width 4?

This is PIO, limited by the x86 address space to 16 bit. Will add a comment.

Quoted 1 line

Why not use access_with_adjusted_size()?

Because of different accessor prototypes.

Quoted 32 lines · Jan Kiszka

Quoted 31 lines · Avi Kivity

Quoted 29 lines · Jan Kiszka

From: Jan Kiszka<[email protected]>

As we register old portio regions via ioport_register, we are also responsible for providing the word access wrapper.

Signed-off-by Jan Kiszka<[email protected]>

---

Oops, was lacking a shift for word reads.

   memory.c |   10 ++++++++++
   1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index b3ee232..aef4702 100644
--- a/memory.c
+++ b/memory.c
@@ -397,6 +397,11 @@ static void memory_region_iorange_read(IORange *iorange,

           *data = ((uint64_t)1<<   (width * 8)) - 1;
           if (mrp) {
               *data = mrp->read(mr->opaque, offset + mr->offset);
 +        } else if (width == 2) {
 +            mrp = find_portio(mr, offset, 1, false);
 +            assert(mrp);
 +            *data = mrp->read(mr->opaque, offset + mr->offset) |
 +                    (mrp->read(mr->opaque, offset + mr->offset +
 1)<<   8);
           }

Quoted 4 lines

Quoted 1 line

What about width 4?

This is PIO, limited by the x86 address space to 16 bit. Will add a comment.

x86 PIO is not limited to 16 bits, just ISA, which memory.c knows nothing about.

Quoted 5 lines

Quoted 2 lines

Why not use access_with_adjusted_size()?

Because of different accessor prototypes.

Can be thunked. There is a different issue, a_w_a_s() can use small accesses to emulate large ones, but not vice versa. It needs fixing anyway.

Quoted 6 lines

Quoted 3 lines

This is PIO, limited by the x86 address space to 16 bit. Will add a comment.

x86 PIO is not limited to 16 bits, just ISA, which memory.c knows nothing about.

Confused address and data, the former is limited 16, the latter can be 32 as well. But I guess only ISA models made use of the core's split up service, and that's why QEMU limited itself accordingly.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

Why not use access_with_adjusted_size()?

Because of different accessor prototypes.

Quoted 4 lines

Can be thunked. There is a different issue, a_w_a_s() can use small accesses to emulate large ones, but not vice versa. It needs fixing anyway.

IIRC, that's a feature: Devices not implementing small accesses tend to refuse them in reality.

Quoted 4 lines · Jan Kiszka

Quoted 4 lines

Quoted 3 lines

This is PIO, limited by the x86 address space to 16 bit. Will add a comment.

Quoted 6 lines

Quoted 2 lines

x86 PIO is not limited to 16 bits, just ISA, which memory.c knows nothing about.

Confused address and data, the former is limited 16, the latter can be 32 as well. But I guess only ISA models made use of the core's split up service, and that's why QEMU limited itself accordingly.

Let's not bury such details in the core.

Quoted 13 lines

Quoted 12 lines

Quoted 7 lines

Quoted 4 lines

Why not use access_with_adjusted_size()?

Because of different accessor prototypes.

Can be thunked. There is a different issue, a_w_a_s() can use small accesses to emulate large ones, but not vice versa. It needs fixing anyway.

Quoted 2 lines

IIRC, that's a feature: Devices not implementing small accesses tend to refuse them in reality.

I don't think this holds for pci; there the bus always generates 32-bit writes with separate byte enables for each lane. The device need not even be aware of a sub-word access, for reads.

Quoted 8 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka

Quoted 6 lines

Quoted 3 lines

This is PIO, limited by the x86 address space to 16 bit. Will add a comment.

x86 PIO is not limited to 16 bits, just ISA, which memory.c knows nothing about.

Quoted 5 lines

Quoted 3 lines

Confused address and data, the former is limited 16, the latter can be 32 as well. But I guess only ISA models made use of the core's split up service, and that's why QEMU limited itself accordingly.

Let's not bury such details in the core.

It's already in the core (ioport), and would refrain from changing it in this fix.

Quoted 4 lines

I don't think this holds for pci; there the bus always generates 32-bit writes with separate byte enables for each lane. The device need not even be aware of a sub-word access, for reads.

The problem is that once we "enhance" the core with such a support to potentially help one use case, we need to validate all users again if they depend on the old behavior. That's tricky as breakage may only show up with odd guests that issue invalid but so far harmless requests.

Quoted 14 lines · Jan Kiszka

Quoted 3 lines · Avi Kivity

Quoted 2 lines · Jan Kiszka

Quoted 2 lines

Quoted 2 lines

This is PIO, limited by the x86 address space to 16 bit. Will

add a

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

comment.

x86 PIO is not limited to 16 bits, just ISA, which memory.c knows nothing about.

Confused address and data, the former is limited 16, the latter

can be

Quoted 1 line

Quoted 1 line

32 as well. But I guess only ISA models made use of the core's

split up

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

service, and that's why QEMU limited itself accordingly.

Let's not bury such details in the core.

It's already in the core (ioport), and would refrain from changing it in this fix.

That's the bad old core we're trying to get away from.

Quoted 11 lines

Quoted 3 lines

I don't think this holds for pci; there the bus always generates

32-bit

Quoted 2 lines

writes with separate byte enables for each lane. The device need not even be aware of a sub-word access, for reads.

The problem is that once we "enhance" the core with such a support to potentially help one use case, we need to validate all users again if they depend on the old behavior. That's tricky as breakage may only show up with odd guests that issue invalid but so far harmless requests.

It's opt-in. If a device sets MemoryRegionOps::impl.{min,max}_access_size = 1, it will only be fed byte accesses (the core will take care of breaking apart larger writes). If it sets MemoryRegionOps::impl.{min,max}_access_size = 4, it will only get long accesses (and the core will/should shift/mask or RMW). Refusing illegal access sizes is done using MemoryRegionOps::valid. Most of this is unimplemented unfortunately.

Quoted 7 lines

It's opt-in. If a device sets MemoryRegionOps::impl.{min,max}_access_size = 1, it will only be fed byte accesses (the core will take care of breaking apart larger writes). If it sets MemoryRegionOps::impl.{min,max}_access_size = 4, it will only get long accesses (and the core will/should shift/mask or RMW). Refusing illegal access sizes is done using MemoryRegionOps::valid. Most of this is unimplemented unfortunately.

That makes sense (for non-old_portio users).

Quoted 4 lines · Jan Kiszka

Quoted 4 lines

It's opt-in. If a device sets MemoryRegionOps::impl.{min,max}_access_size = 1, it will only be fed byte accesses (the core will take care of breaking apart larger writes). If it sets MemoryRegionOps::impl.{min,max}_access_size =

Quoted 8 lines

4, it

Quoted 3 lines

will only get long accesses (and the core will/should shift/mask or RMW). Refusing illegal access sizes is done using MemoryRegionOps::valid. Most of this is unimplemented unfortunately.

That makes sense (for non-old_portio users).

The trick of having a way to register N callbacks with one shot is worth growing. Ideally each register in a BAR would have a callback and we'd do something like

    MemoryRegionOps mydev_ops = {
        .registers = {
             { MYDEV_REG_x, 4, 4, mydev_reg_x_read, mydev_reg_x_write, },
             ...
         },
    }

with hints to the core like "this register sits at this offset, use it for reads instead of a callback", or, "this is a read-only register".

Quoted 12 lines

The trick of having a way to register N callbacks with one shot is worth growing. Ideally each register in a BAR would have a callback and we'd do something like

     MemoryRegionOps mydev_ops = {
         .registers = {
              { MYDEV_REG_x, 4, 4, mydev_reg_x_read, mydev_reg_x_write, },
              ...
          },
     }

Quoted 2 lines

with hints to the core like "this register sits at this offset, use it for reads instead of a callback", or, "this is a read-only register".

This has pros and cons. If you have n registers to dispatch, you then have to write n function prologues and maybe epilogues instead of just one. Specifically if the register access is trivial, that could case quite some LoC blowup on the device side.

What may have a better ratio are generic register get/set handlers.

Quoted 11 lines

The trick of having a way to register N callbacks with one shot is worth growing. Ideally each register in a BAR would have a callback and we'd do something like

     MemoryRegionOps mydev_ops = {
         .registers = {
              { MYDEV_REG_x, 4, 4, mydev_reg_x_read, mydev_reg_x_write, },
              ...
          },
     }

Quoted 2 lines

with hints to the core like "this register sits at this offset, use it for reads instead of a callback", or, "this is a read-only register".

This has pros and cons. If you have n registers to dispatch, you then have to write n function prologues and maybe epilogues instead of just one. Specifically if the register access is trivial, that could case quite some LoC blowup on the device side.

If the register access is trivial then you don't need to call into the driver at all ...

You can have a look at hw/intel-hda.c which actually implements something like this, with some commonly needed features:

* The "offset" field already mentioned by avi is there, so trivial reads/writes can be handled by the core.
* A "wmask" field to specify which bits are guest writable.
* A "wclear" field to specify which bits have write-one-to-clear semantics.
* A "reset" field which specified the value this field has after device reset. Also serves as value for read-only registers.
* read/write handlers of course. The write handler is called after the core applied sanity checks and calculated the new register value (using wmask+wclear).
* A "name" field (for debug logging).

It's pretty nice, alot more readable that a big switch, forces you to think which bits the guest can set (not specifying a wmask gives you a read-only register ;).

Also no bloat. With this moving to memory core the all the handlers will gain a line with a container_of(), but that isn't too bad too IMHO.

From: Jan Kiszka<[email protected]>

As we register old portio regions via ioport_register, we are also responsible for providing the word access wrapper.

Signed-off-by Jan Kiszka<[email protected]>

---

Oops, was lacking a shift for word reads.