qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] memory: could we add extra input param for memory_regio


From: liu ping fan
Subject: Re: [Qemu-devel] memory: could we add extra input param for memory_region_init_io()?
Date: Fri, 24 Aug 2012 17:47:49 +0800

On Tue, Aug 21, 2012 at 8:41 PM, Avi Kivity <address@hidden> wrote:
> On 08/21/2012 02:18 PM, liu ping fan wrote:
>>>
>>>> But as
>>>> it will also take the code path which has object_ref(Object*), so it
>>>> has to convert, otherwise the code will corrupt.
>>>> That is what I want to express.
>>>
>>> Option 2, for example, had MemoryRegionOps::object(MemoryRegion *) which
>>> can be used to convert the opaque to an Object, or return NULL.  With
>>> that option, there would be no corruption:
>>>
>> If we did not pass extra flag to indicate whether opaque is Object or
>> not, we can not implement MemoryRegionOps::object().  Because we lack
>> of something like "try,catch", and object_dynamic_cast() can not work.
>> So besides memory_region_init_io(), we need extra interface to pass that 
>> flag?
>
> The interface is MemoryRegionOps::object().
>
> If the user does not implement it, then the opaque cannot be converted
> into an object.  If the user did implement it, then the function
> specifies how to convert it.
>
> For example:
>
>
> static Object *e1000_mmio_object(void *opaque)
> {
>     E1000State *s = opaque;
>
>     return &s->dev.qdev.parent_obj;
> }
>
> static const MemoryRegionOps e1000_mmio_ops = {
>     .read = e1000_mmio_read,
>     .write = e1000_mmio_write,
>     .endianness = DEVICE_LITTLE_ENDIAN,
>     .object = e1000_mmio_object,
>     .impl = {
>         .min_access_size = 4,
>         .max_access_size = 4,
>     },
> };
>
>>>   qemu_mutex_lock(&mem_map_lock);
>>>   MemoryRegionSection mrs = walk(&phys_map);
>>>   Object *object = mrs.mr->ops->object(mrs.mr);
>>>   if (object) {
>>>       object_ref(object);
>>>   }
>>>   qemu_mutex_unlock(&mem_map_unlock);
>>>
>
> This should read:
>
>
>     Object *object = NULL;
>     if (mrs.mr->ops->object) {

Oh, thanks. I have not realize it at the first glance.  Thank you for
the great patient.
During these days, when I try to verify the convert,  I run into the
issue similar as you point out for "assigned device can disappear
while one of its BARs remain".   For example,
typedef struct PCIIDEState {
    PCIDevice dev;
    IDEBus bus[2]; ==> lies in parent obj,
..........................
}
It is an create-in-place issue,
void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id)
{
    qbus_create_inplace(&idebus->qbus, TYPE_IDE_BUS, dev, NULL);
    idebus->bus_id = bus_id;
}

When I try to fix such issue, I got another patchset "Subject: [PATCH
0/10] rework on hot unplug",  in a short word, unplug will break the
refer circular, so we can have back ref from IDEBus to PCIIDEState
will CC you.

Thanks and regards,
pingfan

>         object = mrs.mr->ops->object(mrs.mr);
>     }
>
>
>>> So there is no memory corruption.  However, as I point out below, we
>>> also lack the reference count.  Maybe we could do something like:
>>>
>> I think the hotplug issue is only for limited type of bus. And
>> fortunately, for pci, they have already adopt Object.
>> So for other SoC, maybe we can ignore this issue at current step
>
> We still have to take the big qemu lock somehow.
>
>>> If we can avoid a cycle.  Won't the device need to hold refs to the BAR?
>>
>> I will check the code, but I has thought BAR is implemented by
>> assigned device?
>
> Yes.
>
>
> --
> error compiling committee.c: too many arguments to function



reply via email to

[Prev in Thread] Current Thread [Next in Thread]