Re: [Qemu-devel] [PATCH 03/15] hw/intc/arm_gic: Add Security Extensions property

[Sergey Fedorov]

On 25.08.2014 13:39, Aggeler Fabian wrote:
> On 25 Aug 2014, at 11:20, Sergey Fedorov <address@hidden> wrote:
>
>> On 22.08.2014 14:29, Fabian Aggeler wrote:
>>> The existing implementation does not support Security Extensions mentioned
>>> in the GICv1 and GICv2 architecture specification. Security Extensions are
>>> not available on all GICs. This property makes it possible to enable 
>>> Security Extensions.
>>>
>>> It also makes GICD_TYPER/ICDICTR.SecurityExtn RAO for GICs which implement
>>> Security Extensions.
>>>
>>> Signed-off-by: Fabian Aggeler <address@hidden>
>>> ---
>>> hw/intc/arm_gic.c                | 5 ++++-
>>> hw/intc/arm_gic_common.c         | 1 +
>>> include/hw/intc/arm_gic_common.h | 1 +
>>> 3 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>>> index b27bd0e..75b5121 100644
>>> --- a/hw/intc/arm_gic.c
>>> +++ b/hw/intc/arm_gic.c
>>> @@ -297,7 +297,10 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr 
>>> offset)
>>>         if (offset == 0)
>>>             return s->enabled;
>>>         if (offset == 4)
>>> -            return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5);
>>> +            /* Interrupt Controller Type Register */
>>> +            return ((s->num_irq / 32) - 1)
>>> +                    | ((NUM_CPU(s) - 1) << 5)
>>> +                    | (s->security_extn << 10);
>>>         if (offset < 0x08)
>>>             return 0;
>>>         if (offset >= 0x80) {
>>> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
>>> index 6d884ec..302a056 100644
>>> --- a/hw/intc/arm_gic_common.c
>>> +++ b/hw/intc/arm_gic_common.c
>>> @@ -149,6 +149,7 @@ static Property arm_gic_common_properties[] = {
>>>      * (Internally, 0xffffffff also indicates "not a GIC but an NVIC".)
>>>      */
>>>     DEFINE_PROP_UINT32("revision", GICState, revision, 1),
>>> +    DEFINE_PROP_UINT8("security-extn", GICState, security_extn, 0),
>> Why don't use bool type and DEFINE_PROP_BOOL for this field?
>>
>> Regards,
>> Sergey.
> I used an uint8 to be able to shift the security_extn field in the return 
> value of the
> Interrupt Controller Type Register (see above). 

But uint8 type wouldn't be enough to shift 10 bits left, isn't it?
AFAIU, integer promotion should convert bool to int in this case.

Best,
Sergey

>
> @Edgar: how did you add Virtualization Extensions to the GIC implementation? 
> Does it make sense to combine the extensions into one QOM property?
>
> Best,
> Fabian
>
>>>     DEFINE_PROP_END_OF_LIST(),
>>> };
>>>
>>> diff --git a/include/hw/intc/arm_gic_common.h 
>>> b/include/hw/intc/arm_gic_common.h
>>> index 01c6f24..4e25017 100644
>>> --- a/include/hw/intc/arm_gic_common.h
>>> +++ b/include/hw/intc/arm_gic_common.h
>>> @@ -105,6 +105,7 @@ typedef struct GICState {
>>>     MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
>>>     uint32_t num_irq;
>>>     uint32_t revision;
>>> +    uint8_t security_extn;
>>>     int dev_fd; /* kvm device fd if backed by kvm vgic support */
>>> } GICState;