Re: [Qemu-devel] [PATCH 09/24] omap_gpmc/nseries/tusb6010: convert to memory API
On 08/08/2011 08:43 PM, Peter Maydell wrote: On 8 August 2011 18:06, Avi Kivitya...@redhat.com wrote: Somewhat clumsy since it needs a variable sized region. @@ -119,7 +120,7 @@ void omap_sdrc_reset(struct omap_sdrc_s *s); struct omap_gpmc_s; struct omap_gpmc_s *omap_gpmc_init(target_phys_addr_t base, qemu_irq irq); void omap_gpmc_reset(struct omap_gpmc_s *s); -void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, int iomemtype, +void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, MemoryRegion *iomem, void (*base_upd)(void *opaque, target_phys_addr_t new), void (*unmap)(void *opaque), void *opaque); I have a pile of omap_gpmc patches almost ready to submit which are mostly waiting on my figuring out how to get them to work with the new memory API. In particular I think this is wrong -- omap_gpmc_attach should just take a MemoryRegion*, and there should be standard APIs for resize this memory region and unmap this memory region, [we have the latter but not the former currently], at which point there's no need to pass in base_upd or unmap function pointers. I agree we need better support for this in the core. But I'm limiting core updates so I can continue to convert users, without which the entire effort is useless, and so I can collect additional requirements. Also, my patchset focuses on mechanical transformations. It is already risky enough in terms of regressions, I'm not going to rewrite/improve all of qemu; if you want those callbacks removed, you will have to remove them yourself. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Re: [Qemu-devel] [PATCH 09/24] omap_gpmc/nseries/tusb6010: convert to memory API
On 9 August 2011 07:34, Avi Kivity a...@redhat.com wrote: Also, my patchset focuses on mechanical transformations. It is already risky enough in terms of regressions, I'm not going to rewrite/improve all of qemu; if you want those callbacks removed, you will have to remove them yourself. Sure. Can I ask you to drop this one and the onenand patch, and I'll have a go over the next week or three at incorporating a conversion into the patchset I have for those? Thanks -- PMM
Re: [Qemu-devel] [PATCH 09/24] omap_gpmc/nseries/tusb6010: convert to memory API
On 08/09/2011 10:37 AM, Peter Maydell wrote: On 9 August 2011 07:34, Avi Kivitya...@redhat.com wrote: Also, my patchset focuses on mechanical transformations. It is already risky enough in terms of regressions, I'm not going to rewrite/improve all of qemu; if you want those callbacks removed, you will have to remove them yourself. Sure. Can I ask you to drop this one and the onenand patch, and I'll have a go over the next week or three at incorporating a conversion into the patchset I have for those? Umm... next week or three? I'd like to move fast on this. Can't you base your patches on this instead? After all, this is just a mechanical transformation, there should be no functional change. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Re: [Qemu-devel] [PATCH 09/24] omap_gpmc/nseries/tusb6010: convert to memory API
On 9 August 2011 08:41, Avi Kivity a...@redhat.com wrote:
On 08/09/2011 10:37 AM, Peter Maydell wrote:
On 9 August 2011 07:34, Avi Kivitya...@redhat.com wrote:
Also, my patchset focuses on mechanical transformations. It is already
risky enough in terms of regressions, I'm not going to rewrite/improve
all
of qemu; if you want those callbacks removed, you will have to remove
them
yourself.
Sure. Can I ask you to drop this one and the onenand patch, and I'll
have a go over the next week or three at incorporating a conversion into
the patchset I have for those?
Umm... next week or three? I'd like to move fast on this.
I'm busy at least half of this week, and next week is the KVM
Forum, so I didn't want to promise anything faster.
Can't you base your patches on this instead? After all, this is just a
mechanical transformation, there should be no functional change.
Well, maybe, but it seems a bit silly to commit something which is
going to be half-reverted and rewritten.
Also I just noticed this change:
-static CPUReadMemoryFunc * const omap_gpmc_readfn[] = {
-omap_badwidth_read32, /* TODO */
-omap_badwidth_read32, /* TODO */
-omap_gpmc_read,
-};
-
-static CPUWriteMemoryFunc * const omap_gpmc_writefn[] = {
-omap_badwidth_write32, /* TODO */
-omap_badwidth_write32, /* TODO */
-omap_gpmc_write,
+static const MemoryRegionOps omap_gpmc_ops = {
+/* TODO: specialize 4 byte writes? */
+.read = omap_gpmc_read,
+.write = omap_gpmc_write,
+.endianness = DEVICE_NATIVE_ENDIAN,
};
...isn't this just throwing away the warnings on bad-width accesses?
(it's not clear to me from the docs what the behaviour of the new
API on bad-width accesses is going to be.)
-- PMM
Re: [Qemu-devel] [PATCH 09/24] omap_gpmc/nseries/tusb6010: convert to memory API
On 08/09/2011 11:07 AM, Peter Maydell wrote:
On 9 August 2011 08:41, Avi Kivitya...@redhat.com wrote:
On 08/09/2011 10:37 AM, Peter Maydell wrote:
On 9 August 2011 07:34, Avi Kivitya...@redhat.comwrote:
Also, my patchset focuses on mechanical transformations. It is already
risky enough in terms of regressions, I'm not going to rewrite/improve
all
of qemu; if you want those callbacks removed, you will have to remove
them
yourself.
Sure. Can I ask you to drop this one and the onenand patch, and I'll
have a go over the next week or three at incorporating a conversion into
the patchset I have for those?
Umm... next week or three? I'd like to move fast on this.
I'm busy at least half of this week, and next week is the KVM
Forum, so I didn't want to promise anything faster.
Can't you base your patches on this instead? After all, this is just a
mechanical transformation, there should be no functional change.
Well, maybe, but it seems a bit silly to commit something which is
going to be half-reverted and rewritten.
It certainly won't be reverted, since the ram_addr_t based API will be
removed. So it will just be modified to suit however you want it to
be. If you want it otherwise, post a patch, either on top of this or
instead of it, and I will incorporate it into this patchset.
Again, this is not about improving individual devices (that is left for
device maintainers); it's just about converting to the new API with
minimal changes.
Also I just noticed this change:
-static CPUReadMemoryFunc * const omap_gpmc_readfn[] = {
-omap_badwidth_read32, /* TODO */
-omap_badwidth_read32, /* TODO */
-omap_gpmc_read,
-};
-
-static CPUWriteMemoryFunc * const omap_gpmc_writefn[] = {
-omap_badwidth_write32, /* TODO */
-omap_badwidth_write32, /* TODO */
-omap_gpmc_write,
+static const MemoryRegionOps omap_gpmc_ops = {
+/* TODO: specialize4 byte writes? */
+.read = omap_gpmc_read,
+.write = omap_gpmc_write,
+.endianness = DEVICE_NATIVE_ENDIAN,
};
...isn't this just throwing away the warnings on bad-width accesses?
It is; will fix.
(it's not clear to me from the docs what the behaviour of the new
API on bad-width accesses is going to be.)
It's one of the things I'm collecting requirements on. So far I think
we'll have an inheritable, configurable policy that allows you to ignore
writes/read ones, raise a machine check exception, or warn (in developer
mode only).
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH 09/24] omap_gpmc/nseries/tusb6010: convert to memory API
On 08/09/2011 11:44 AM, Avi Kivity wrote:
...isn't this just throwing away the warnings on bad-width accesses?
It is; will fix.
Reading the original code, it seems broken:
uint32_t omap_badwidth_read32(void *opaque, target_phys_addr_t addr)
{
uint32_t ret;
OMAP_32B_REG(addr);
cpu_physical_memory_read(addr, (void *) ret, 4);
return ret;
}
The code issues a read from addr, but that is not the original address
used by the guest, since addresses are relative to the start of the
region (in both the old and new APIs), not to physical address start.
So I'll just set access size validity in the new code.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH 09/24] omap_gpmc/nseries/tusb6010: convert to memory API
On 8 August 2011 18:06, Avi Kivity a...@redhat.com wrote: Somewhat clumsy since it needs a variable sized region. @@ -119,7 +120,7 @@ void omap_sdrc_reset(struct omap_sdrc_s *s); struct omap_gpmc_s; struct omap_gpmc_s *omap_gpmc_init(target_phys_addr_t base, qemu_irq irq); void omap_gpmc_reset(struct omap_gpmc_s *s); -void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, int iomemtype, +void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, MemoryRegion *iomem, void (*base_upd)(void *opaque, target_phys_addr_t new), void (*unmap)(void *opaque), void *opaque); I have a pile of omap_gpmc patches almost ready to submit which are mostly waiting on my figuring out how to get them to work with the new memory API. In particular I think this is wrong -- omap_gpmc_attach should just take a MemoryRegion*, and there should be standard APIs for resize this memory region and unmap this memory region, [we have the latter but not the former currently], at which point there's no need to pass in base_upd or unmap function pointers. -- PMM
