From mboxrd@z Thu Jan  1 00:00:00 1970
From: Vijay Kilari <vijay.kilari@gmail.com>
Subject: Re: [PATCH v2 01/15] xen/arm: register mmio handler at
	runtime
Date: Fri, 4 Apr 2014 20:54:27 +0530
Message-ID: <CALicx6tMHbRm7_hBM63XKK_w_UWv72Q3C2XWUdWz2CWY7FS=TQ@mail.gmail.com>
References: <1396612593-443-1-git-send-email-vijay.kilari@gmail.com>
	<1396612593-443-2-git-send-email-vijay.kilari@gmail.com>
	<533EA307.4000900@linaro.org>
	<CALicx6u-6Gz=LcsDjH+Pb_FG2GzXxMHJOqHCZrfDdwBtqR8dQQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CALicx6u-6Gz=LcsDjH+Pb_FG2GzXxMHJOqHCZrfDdwBtqR8dQQ@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Julien Grall <julien.grall@linaro.org>
Cc: Ian Campbell <Ian.Campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Prasun Kapoor <Prasun.Kapoor@caviumnetworks.com>, Vijaya Kumar K <vijaya.kumar@caviumnetworks.com>, xen-devel@lists.xen.org, Stefano Stabellini <stefano.stabellini@citrix.com>
List-Id: xen-devel@lists.xenproject.org

 Hi Julien,

On Fri, Apr 4, 2014 at 6:00 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Fri, Apr 4, 2014 at 5:48 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> Hello vijay,
>>
>> Thank your for the patch.
>>
>> On 04/04/2014 12:56 PM, vijay.kilari@gmail.com wrote:
>>
>> [..]
>>
>>> -#define MMIO_HANDLER_NR ARRAY_SIZE(mmio_handlers)
>>> +static DEFINE_SPINLOCK(handler_lock);
>>
>> Why a global lock rather than a domain lock?
>>
> OK can move into domain io_handler structure
>
>> [..]
>>
>>> +
>>> +void register_mmio_handler(struct domain *d, struct mmio_handler * handle)
>>> +{
>>> +    unsigned long flags;
>>> +    struct io_handler *handler = d->arch.io_handlers;
>>> +    BUG_ON(handler->num_entries >= MAX_IO_HANDLER);
>>> +
>>> +    spin_lock_irqsave(&handler_lock, flags);
>>
>> You don't need to disable the IRQ here.
>   My first version is without IRQ disable. I remember Andrii suggested it
>>
>>> +    handler->mmio_handlers[handler->num_entries++] = handle;
>>> +    spin_unlock_irqrestore(&handler_lock, flags);
>>> +}
>>>  /*
>>>   * Local variables:
>>>   * mode: C
>>> diff --git a/xen/arch/arm/io.h b/xen/arch/arm/io.h
>>> index 8d252c0..5fc1660 100644
>>> --- a/xen/arch/arm/io.h
>>> +++ b/xen/arch/arm/io.h
>>
>> As you are modifying this header. Can you move it in include/asm-arm?
> OK.
>
>>
>>> @@ -40,10 +40,14 @@ struct mmio_handler {
>>>      mmio_write_t write_handler;
>>>  };
>>>
>>> -extern const struct mmio_handler vgic_distr_mmio_handler;
>>> -extern const struct mmio_handler vuart_mmio_handler;
>>> +#define MAX_IO_HANDLER  16
>>> +struct io_handler {
>>> +    int num_entries;
>>> +    struct mmio_handler *mmio_handlers[MAX_IO_HANDLER];
>>> +};
>>>
>>>  extern int handle_mmio(mmio_info_t *info);
>>> +void register_mmio_handler(struct domain *d, struct mmio_handler * handle);
>>
>> As I said on the previous version, it would be nice to remove check
>> callback in the mmio_handler and replace it by addr/size. It's better if
>> we might want to change the place in the memory following the guest.
>>
>> So the result function would be:
>>
>> register_mmio_handler(struct domain *d, read_t read, write_t write,
>> addr, size);
>>
> OK.
   Though you suggested, I kept this check handler because it gives flexibility
for driver to make additional checks if required apart from just checking for
address range.

>>>
>>>  #endif