Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On Tue, Jun 28, 2011 at 03:09:38PM +0300, Avi Kivity wrote: > On 06/28/2011 03:07 PM, Jan Kiszka wrote: > > > > > > The point is that different buses have different widths. > > > target_phys_addr_t matches just one bus in the system. It needs to be > > > the maximum size of all buses present to be useful. > > > > Then we need a type for that. Or we need to demand that > > target_phys_addr_t is defined large enough to support all buses that the > > particular arch wants to address. Hardcoding 64 bit or anything is not > > appropriate for a generic subsystem. > > Okay, let's make t_p_a_t max(bus size in system). Do we have 32-bit > targets that don't support pci (I guess, pc-isa with cpu < ppro?). Do > we want to support a 32-bit variant of pci? It certainly existed at > some point. PCI always had a mechanism for 64-bits addresses even on 32-bits wide bus, called Dual Address Cycle. I'm not sure which was rarer: devices which could handle it, or north bridges which could use it. Probably a tie. But in theory, it was there. OG.
Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On 06/28/2011 04:25 PM, Peter Maydell wrote: On 28 June 2011 13:09, Avi Kivity wrote: > Okay, let's make t_p_a_t max(bus size in system). If you want a type for that, can't you give it a sensible (ie different) name? target_phys_addr_t is pretty clearly "the type of a physical address for this target" and having it actually be something else is just going to be confusing. "a physical address" is ambiguous. There are many physical addresses flowing around. Certainly it's most natural to think about the processor's physical address bus, but that's not always useful. Since all *devices* use target_phys_addr_t, I think we should just adopt that to avoid major and pointless churn. > Do we have 32-bit targets > that don't support pci (I guess, pc-isa with cpu< ppro?). Do we want to > support a 32-bit variant of pci? It certainly existed at some point. As a thought experiment, you could take an existing 32 bit target and define a new board model that happens to have eg a new pci controller on it. It doesn't seem right that that should cause the system's idea of this type width to change, it's just a new device model and board. So if you have this type I think it ought to be max(bus size of widest bus qemu supports). That indicates typedef uint64_t target_phys_addr_t; -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On 28 June 2011 13:09, Avi Kivity wrote: > Okay, let's make t_p_a_t max(bus size in system). If you want a type for that, can't you give it a sensible (ie different) name? target_phys_addr_t is pretty clearly "the type of a physical address for this target" and having it actually be something else is just going to be confusing. > Do we have 32-bit targets > that don't support pci (I guess, pc-isa with cpu < ppro?). Do we want to > support a 32-bit variant of pci? It certainly existed at some point. As a thought experiment, you could take an existing 32 bit target and define a new board model that happens to have eg a new pci controller on it. It doesn't seem right that that should cause the system's idea of this type width to change, it's just a new device model and board. So if you have this type I think it ought to be max(bus size of widest bus qemu supports). -- PMM
Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On 06/28/2011 03:46 PM, Jan Kiszka wrote: > Do we want to support a 32-bit variant of pci? It certainly existed at > some point. As long as making everything 64 bit in the implementation of the device models is not guest visible, I don't think that should be a problem. How would it become guest visible? AFAICT the only implication is a very minor slowdown in the cases where it is not actually required. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On 2011-06-28 14:09, Avi Kivity wrote: > On 06/28/2011 03:07 PM, Jan Kiszka wrote: >>> >>> The point is that different buses have different widths. >>> target_phys_addr_t matches just one bus in the system. It needs to be >>> the maximum size of all buses present to be useful. >> >> Then we need a type for that. Or we need to demand that >> target_phys_addr_t is defined large enough to support all buses that the >> particular arch wants to address. Hardcoding 64 bit or anything is not >> appropriate for a generic subsystem. > > Okay, let's make t_p_a_t max(bus size in system). Do we have 32-bit > targets that don't support pci (I guess, pc-isa with cpu < ppro?). At least lm32 and microblaze appear to fall into that category. > Do we want to support a 32-bit variant of pci? It certainly existed at > some point. As long as making everything 64 bit in the implementation of the device models is not guest visible, I don't think that should be a problem. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On 06/28/2011 03:07 PM, Jan Kiszka wrote: > > The point is that different buses have different widths. > target_phys_addr_t matches just one bus in the system. It needs to be > the maximum size of all buses present to be useful. Then we need a type for that. Or we need to demand that target_phys_addr_t is defined large enough to support all buses that the particular arch wants to address. Hardcoding 64 bit or anything is not appropriate for a generic subsystem. Okay, let's make t_p_a_t max(bus size in system). Do we have 32-bit targets that don't support pci (I guess, pc-isa with cpu < ppro?). Do we want to support a 32-bit variant of pci? It certainly existed at some point. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On 2011-06-28 13:53, Avi Kivity wrote:
> On 06/28/2011 01:28 PM, Jan Kiszka wrote:
>> On 2011-06-28 12:03, Michael S. Tsirkin wrote:
+struct MemoryRegion {
+/* All fields are private - violators will be prosecuted */
+const MemoryRegionOps *ops;
+MemoryRegion *parent;
+uint64_t size;
+target_phys_addr_t addr;
+target_phys_addr_t offset;
+ram_addr_t ram_addr;
+bool has_ram_addr;
+MemoryRegion *alias;
+target_phys_addr_t alias_offset;
+unsigned priority;
+bool may_overlap;
+QTAILQ_HEAD(subregions, MemoryRegion) subregions;
+QTAILQ_ENTRY(MemoryRegion) subregions_link;
+QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
+const char *name;
>>>
>>> I'm never completely sure whether these should be target addresses
>>> or bus addresses or just uint64_t.
>>> With pci on a 32 bit system you can stick a 64 bit address
>>> in a BAR and the result will be that it is never accessed
>>> from the CPU.
>>>
>>
>> Memory regions are not bound to any current or future PCI
>> specifications. Any fixed bit width would be wrong here, ie. size should
>> rather be target_phys_addr_t.
>
> The point is that different buses have different widths.
> target_phys_addr_t matches just one bus in the system. It needs to be
> the maximum size of all buses present to be useful.
Then we need a type for that. Or we need to demand that
target_phys_addr_t is defined large enough to support all buses that the
particular arch wants to address. Hardcoding 64 bit or anything is not
appropriate for a generic subsystem.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On 06/28/2011 01:28 PM, Jan Kiszka wrote:
On 2011-06-28 12:03, Michael S. Tsirkin wrote:
>> +struct MemoryRegion {
>> +/* All fields are private - violators will be prosecuted */
>> +const MemoryRegionOps *ops;
>> +MemoryRegion *parent;
>> +uint64_t size;
>> +target_phys_addr_t addr;
>> +target_phys_addr_t offset;
>> +ram_addr_t ram_addr;
>> +bool has_ram_addr;
>> +MemoryRegion *alias;
>> +target_phys_addr_t alias_offset;
>> +unsigned priority;
>> +bool may_overlap;
>> +QTAILQ_HEAD(subregions, MemoryRegion) subregions;
>> +QTAILQ_ENTRY(MemoryRegion) subregions_link;
>> +QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
>> +const char *name;
>
> I'm never completely sure whether these should be target addresses
> or bus addresses or just uint64_t.
> With pci on a 32 bit system you can stick a 64 bit address
> in a BAR and the result will be that it is never accessed
> from the CPU.
>
Memory regions are not bound to any current or future PCI
specifications. Any fixed bit width would be wrong here, ie. size should
rather be target_phys_addr_t.
The point is that different buses have different widths.
target_phys_addr_t matches just one bus in the system. It needs to be
the maximum size of all buses present to be useful.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On 06/28/2011 01:03 PM, Michael S. Tsirkin wrote:
On Mon, Jun 27, 2011 at 04:21:48PM +0300, Avi Kivity wrote:
...
> +static bool memory_region_access_valid(MemoryRegion *mr,
> + target_phys_addr_t addr,
> + unsigned size)
> +{
> +if (!mr->ops->valid.unaligned&& (addr& (size - 1))) {
> +return false;
> +}
> +
> +/* Treat zero as compatibility all valid */
> +if (!mr->ops->valid.max_access_size) {
> +return true;
> +}
> +
> +if (size> mr->ops->valid.max_access_size
> +|| size< mr->ops->valid.min_access_size) {
> +return false;
> +}
> +return true;
> +}
> +
> +static uint32_t memory_region_read_thunk_n(void *_mr,
> + target_phys_addr_t addr,
> + unsigned size)
> +{
> +MemoryRegion *mr = _mr;
> +unsigned access_size, access_size_min, access_size_max;
> +uint64_t access_mask;
> +uint32_t data = 0, tmp;
> +unsigned i;
> +
> +if (!memory_region_access_valid(mr, addr, size)) {
> +return -1U; /* FIXME: better signalling */
> +}
> +
> +/* FIXME: support unaligned access */
> +
> +access_size_min = mr->ops->impl.max_access_size;
min = max: Intentional? Cut&paste error?
Bug; thanks.
> diff --git a/memory.h b/memory.h
> new file mode 100644
> index 000..a67ff94
> --- /dev/null
> +++ b/memory.h
> @@ -0,0 +1,201 @@
> +#ifndef MEMORY_H
> +#define MEMORY_H
> +
> +#ifndef CONFIG_USER_ONLY
What's the story with this ifdef?
There are no stubs provided ...
No callers either - I build with a full configuration. I prefer the
#ifdef here rather than all call sites.
> +
> +struct MemoryRegion {
> +/* All fields are private - violators will be prosecuted */
> +const MemoryRegionOps *ops;
> +MemoryRegion *parent;
> +uint64_t size;
> +target_phys_addr_t addr;
> +target_phys_addr_t offset;
> +ram_addr_t ram_addr;
> +bool has_ram_addr;
> +MemoryRegion *alias;
> +target_phys_addr_t alias_offset;
> +unsigned priority;
> +bool may_overlap;
> +QTAILQ_HEAD(subregions, MemoryRegion) subregions;
> +QTAILQ_ENTRY(MemoryRegion) subregions_link;
> +QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
> +const char *name;
I'm never completely sure whether these should be target addresses
or bus addresses or just uint64_t.
With pci on a 32 bit system you can stick a 64 bit address
in a BAR and the result will be that it is never accessed
from the CPU.
I agree. Anyone objects to making the memory API 64-bit?
It will reduce performance slightly for 32-on-32, but these
configurations are getting rarer, and the performance loss is quite small.
Maybe we should make t_p_a_t 64-bit unconditionally. Note that sizes
have to be 64-bit in any case, otherwise you can't express a 4G range
without tricks.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On 2011-06-28 12:03, Michael S. Tsirkin wrote:
>> +struct MemoryRegion {
>> +/* All fields are private - violators will be prosecuted */
>> +const MemoryRegionOps *ops;
>> +MemoryRegion *parent;
>> +uint64_t size;
>> +target_phys_addr_t addr;
>> +target_phys_addr_t offset;
>> +ram_addr_t ram_addr;
>> +bool has_ram_addr;
>> +MemoryRegion *alias;
>> +target_phys_addr_t alias_offset;
>> +unsigned priority;
>> +bool may_overlap;
>> +QTAILQ_HEAD(subregions, MemoryRegion) subregions;
>> +QTAILQ_ENTRY(MemoryRegion) subregions_link;
>> +QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
>> +const char *name;
>
> I'm never completely sure whether these should be target addresses
> or bus addresses or just uint64_t.
> With pci on a 32 bit system you can stick a 64 bit address
> in a BAR and the result will be that it is never accessed
> from the CPU.
>
Memory regions are not bound to any current or future PCI
specifications. Any fixed bit width would be wrong here, ie. size should
rather be target_phys_addr_t.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC v2 01/20] Hierarchical memory region API
On Mon, Jun 27, 2011 at 04:21:48PM +0300, Avi Kivity wrote:
...
> +static bool memory_region_access_valid(MemoryRegion *mr,
> + target_phys_addr_t addr,
> + unsigned size)
> +{
> +if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
> +return false;
> +}
> +
> +/* Treat zero as compatibility all valid */
> +if (!mr->ops->valid.max_access_size) {
> +return true;
> +}
> +
> +if (size > mr->ops->valid.max_access_size
> +|| size < mr->ops->valid.min_access_size) {
> +return false;
> +}
> +return true;
> +}
> +
> +static uint32_t memory_region_read_thunk_n(void *_mr,
> + target_phys_addr_t addr,
> + unsigned size)
> +{
> +MemoryRegion *mr = _mr;
> +unsigned access_size, access_size_min, access_size_max;
> +uint64_t access_mask;
> +uint32_t data = 0, tmp;
> +unsigned i;
> +
> +if (!memory_region_access_valid(mr, addr, size)) {
> +return -1U; /* FIXME: better signalling */
> +}
> +
> +/* FIXME: support unaligned access */
> +
> +access_size_min = mr->ops->impl.max_access_size;
min = max: Intentional? Cut&paste error?
> +if (!access_size_min) {
> +access_size_min = 1;
> +}
> +access_size_max = mr->ops->impl.max_access_size;
> +if (!access_size_max) {
> +access_size_max = 4;
> +}
...
> diff --git a/memory.h b/memory.h
> new file mode 100644
> index 000..a67ff94
> --- /dev/null
> +++ b/memory.h
> @@ -0,0 +1,201 @@
> +#ifndef MEMORY_H
> +#define MEMORY_H
> +
> +#ifndef CONFIG_USER_ONLY
What's the story with this ifdef?
There are no stubs provided ...
> +
> +#include
> +#include
> +#include "qemu-common.h"
> +#include "cpu-common.h"
> +#include "targphys.h"
> +#include "qemu-queue.h"
> +
> +typedef struct MemoryRegionOps MemoryRegionOps;
> +typedef struct MemoryRegion MemoryRegion;
> +
> +/* Must match *_DIRTY_FLAGS in cpu-all.h. To be replaced with dynamic
> + * registration.
> + */
> +#define DIRTY_MEMORY_VGA 0
> +#define DIRTY_MEMORY_CODE 1
> +#define DIRTY_MEMORY_MIGRATION 3
> +
> +/*
> + * Memory region callbacks
> + */
> +struct MemoryRegionOps {
> +/* Read from the memory region. @addr is relative to @mr; @size is
> + * in bytes. */
> +uint64_t (*read)(MemoryRegion *mr,
> + target_phys_addr_t addr,
> + unsigned size);
> +/* Write to the memory region. @addr is relative to @mr; @size is
> + * in bytes. */
> +void (*write)(MemoryRegion *mr,
> + target_phys_addr_t addr,
> + uint64_t data,
> + unsigned size);
> +
> +enum device_endian endianness;
> +/* Guest-visible constraints: */
> +struct {
> +/* If nonzero, specify bounds on access sizes beyond which a machine
> + * check is thrown.
> + */
> +unsigned min_access_size;
> +unsigned max_access_size;
> +/* If true, unaligned accesses are supported. Otherwise unaligned
> + * accesses throw machine checks.
> + */
> + bool unaligned;
> +} valid;
> +/* Internal implementation constraints: */
> +struct {
> +/* If nonzero, specifies the minimum size implemented. Smaller sizes
> + * will be rounded upwards and a partial result will be returned.
> + */
> +unsigned min_access_size;
> +/* If nonzero, specifies the maximum size implemented. Larger sizes
> + * will be done as a series of accesses with smaller sizes.
> + */
> +unsigned max_access_size;
> +/* If true, unaligned accesses are supported. Otherwise all accesses
> + * are converted to (possibly multiple) naturally aligned accesses.
> + */
> + bool unaligned;
> +} impl;
> +};
> +
> +typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> +
> +struct MemoryRegion {
> +/* All fields are private - violators will be prosecuted */
> +const MemoryRegionOps *ops;
> +MemoryRegion *parent;
> +uint64_t size;
> +target_phys_addr_t addr;
> +target_phys_addr_t offset;
> +ram_addr_t ram_addr;
> +bool has_ram_addr;
> +MemoryRegion *alias;
> +target_phys_addr_t alias_offset;
> +unsigned priority;
> +bool may_overlap;
> +QTAILQ_HEAD(subregions, MemoryRegion) subregions;
> +QTAILQ_ENTRY(MemoryRegion) subregions_link;
> +QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
> +const char *name;
I'm never completely sure whether these should be target addresses
or bus addresses or just uint64_t.
With pci on a 32 bit system you can stick a 64 bit address
in a BAR and the result will be that it is never accessed
from the CPU.
--
MST
