On 17/12/15 10:10, Shannon Zhao wrote:
>
>
> On 2015/12/17 17:38, Marc Zyngier wrote:
>> On 17/12/15 08:41, Shannon Zhao wrote:
>>>>
>>>>
>>>> On 2015/12/17 16:33, Marc Zyngier wrote:
>>>>>> On Thu, 17 Dec 2015 15:22:50 +0800
>>>>>> Shannon Zhao <zhaoshengl...@huawei.com> wrote:
>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2015/12/17 4:33, Christoffer Dall wrote:
>>>>>>>>>>>>>> On Wed, Dec 16, 2015 at 04:06:49PM +0800, Shannon Zhao wrote:
>>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 2015/12/16 15:31, Shannon Zhao wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> But in this case, you're
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> returning an error if it is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> *not* initialized.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I understand that in that case
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> you cannot return an interrupt
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> number (-1
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> would be weird), but returning
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -EBUSY feels even more weird.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'd settle for -ENOXIO, or
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> something similar. Anyone having
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> a better idea?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ENXIO or ENODEV would be my choice too, and add
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that to the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Documentation clearly describing when this error
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> code is used.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> By the way, why do you loop over all VCPUS to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> set the same value when
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> you can't do anything per VCPU anyway? It seems
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to me it's either a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> per-VM property (that you can store on the VM
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> data structure) or it's a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> true per-VCPU property?
>>>>>>>>>>>>>>>>>>>>>> This is a per-VCPU property. PMU interrupt could be PPI
>>>>>>>>>>>>>>>>>>>>>> or SPI. For PPI
>>>>>>>>>>>>>>>>>>>>>> the interrupt numbers are same for each vcpu, while for
>>>>>>>>>>>>>>>>>>>>>> SPI they are
>>>>>>>>>>>>>>>>>>>>>> different, so it needs to set them separately. I planned
>>>>>>>>>>>>>>>>>>>>>> to support both
>>>>>>>>>>>>>>>>>>>>>> PPI and SPI. I think I should add support for SPI at
>>>>>>>>>>>>>>>>>>>>>> this moment and let
>>>>>>>>>>>>>>>>>>>>>> users (QEMU) to set these interrupts for each one.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> How about below vPMU Documentation?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> ARM Virtual Performance Monitor Unit (vPMU)
>>>>>>>>>>>>>>>>>> ===========================================
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Device types supported:
>>>>>>>>>>>>>>>>>> KVM_DEV_TYPE_ARM_PMU_V3 ARM Performance Monitor
>>>>>>>>>>>>>>>>>> Unit v3
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Instantiate one PMU instance for per VCPU through this API.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Groups:
>>>>>>>>>>>>>>>>>> KVM_DEV_ARM_PMU_GRP_IRQ
>>>>>>>>>>>>>>>>>> Attributes:
>>>>>>>>>>>>>>>>>> The attr field of kvm_device_attr encodes two values:
>>>>>>>>>>>>>>>>>> bits: | 63 .... 32 | 31 .... 0 |
>>>>>>>>>>>>>>>>>> values: | vcpu_index | irq_num |
>>>>>>>>>> BTW, I change this Attribute to below format and pass vcpu_index
>>>>>>>>>> through
>>>>>>>>>> this Attribute while pass irq_num through kvm_device_attr.addr.
>>>>>>>>>> Is it fine?
>>>>>>>>>>
>>>>>>>>>> bits: | 63 .... 32 | 31 .... 0 |
>>>>>>>>>> values: | reserved | vcpu_index |
>>>>>> Using the .addr field for something that is clearly not an address is
>>>>>> rather odd. Is there any prior usage of that field for something that
>>>>>> is not an address?
>>>>
>>>> I see this usage in vgic_attr_regs_access(). But if you prefer previous
>>>> one, I'll use that.
>> Ah, you're using the .addr field to point to a userspace value, not to
>> carry the value itself! That'd be fine by me (even if I still prefer the
>> original one), but I'd like others to chime in (I'm quite bad at
>> defining userspace stuff...).
>
> Another reason I think is that it needs to pass the irq_num to user
> space when calling get_attr. It could be passed via kvm_device_attr.addr
> while can't be passed via kvm_device_attr.attr within current framework.
That's a very good reason. Go for it.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
