On 06/12/2018 11:16 AM, Greg Kurz wrote:
> On Tue, 12 Jun 2018 16:45:02 +1000
> David Gibson <da...@gibson.dropbear.id.au> wrote:
>
>> From: Cédric Le Goater <c...@kaod.org>
>>
>> The KVM helpers hide the low level interface used to communicate to
>> the XICS KVM device and provide a good cleanup to the XICS KVM models.
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
>> ---
>> hw/intc/xics_kvm.c | 52 +++++++++++++---------------------------------
>> 1 file changed, 14 insertions(+), 38 deletions(-)
>>
>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>> index 89fb20e2c5..8bdf6afe82 100644
>> --- a/hw/intc/xics_kvm.c
>> +++ b/hw/intc/xics_kvm.c
>> @@ -56,10 +56,6 @@ static QLIST_HEAD(, KVMEnabledICP)
>> static void icp_get_kvm_state(ICPState *icp)
>> {
>> uint64_t state;
>> - struct kvm_one_reg reg = {
>> - .id = KVM_REG_PPC_ICP_STATE,
>> - .addr = (uintptr_t)&state,
>> - };
>> int ret;
>>
>> /* ICP for this CPU thread is not in use, exiting */
>> @@ -67,7 +63,7 @@ static void icp_get_kvm_state(ICPState *icp)
>> return;
>> }
>>
>> - ret = kvm_vcpu_ioctl(icp->cs, KVM_GET_ONE_REG, ®);
>> + ret = kvm_get_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE, &state);
>> if (ret != 0) {
>> error_report("Unable to retrieve KVM interrupt controller state"
>> " for CPU %ld: %s", kvm_arch_vcpu_id(icp->cs),
>> strerror(errno));
>> @@ -96,10 +92,6 @@ static void icp_synchronize_state(ICPState *icp)
>> static int icp_set_kvm_state(ICPState *icp, int version_id)
>> {
>> uint64_t state;
>> - struct kvm_one_reg reg = {
>> - .id = KVM_REG_PPC_ICP_STATE,
>> - .addr = (uintptr_t)&state,
>> - };
>> int ret;
>>
>> /* ICP for this CPU thread is not in use, exiting */
>> @@ -111,7 +103,7 @@ static int icp_set_kvm_state(ICPState *icp, int
>> version_id)
>> | ((uint64_t)icp->mfrr << KVM_REG_PPC_ICP_MFRR_SHIFT)
>> | ((uint64_t)icp->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
>>
>> - ret = kvm_vcpu_ioctl(icp->cs, KVM_SET_ONE_REG, ®);
>> + ret = kvm_set_one_reg(icp->cs, KVM_REG_PPC_ICP_STATE, &state);
>> if (ret != 0) {
>> error_report("Unable to restore KVM interrupt controller state (0x%"
>> PRIx64 ") for CPU %ld: %s", state,
>> kvm_arch_vcpu_id(icp->cs),
>> @@ -185,21 +177,15 @@ static const TypeInfo icp_kvm_info = {
>> static void ics_get_kvm_state(ICSState *ics)
>> {
>> uint64_t state;
>> - struct kvm_device_attr attr = {
>> - .flags = 0,
>> - .group = KVM_DEV_XICS_GRP_SOURCES,
>> - .addr = (uint64_t)(uintptr_t)&state,
>> - };
>> int i;
>> + Error *local_err = NULL;
>>
>> for (i = 0; i < ics->nr_irqs; i++) {
>> ICSIRQState *irq = &ics->irqs[i];
>> - int ret;
>> -
>> - attr.attr = i + ics->offset;
>>
>> - ret = ioctl(kernel_xics_fd, KVM_GET_DEVICE_ATTR, &attr);
>> - if (ret != 0) {
>> + kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
>> + i + ics->offset, &state, false, &local_err);
>> + if (local_err) {
>> error_report("Unable to retrieve KVM interrupt controller state"
>> " for IRQ %d: %s", i + ics->offset, strerror(errno));
>> exit(1);
>> @@ -255,19 +241,13 @@ static void ics_synchronize_state(ICSState *ics)
>> static int ics_set_kvm_state(ICSState *ics, int version_id)
>> {
>> uint64_t state;
>> - struct kvm_device_attr attr = {
>> - .flags = 0,
>> - .group = KVM_DEV_XICS_GRP_SOURCES,
>> - .addr = (uint64_t)(uintptr_t)&state,
>> - };
>> int i;
>> + Error *local_err = NULL;
>>
>> for (i = 0; i < ics->nr_irqs; i++) {
>> ICSIRQState *irq = &ics->irqs[i];
>> int ret;
>>
>> - attr.attr = i + ics->offset;
>> -
>> state = irq->server;
>> state |= (uint64_t)(irq->saved_priority & KVM_XICS_PRIORITY_MASK)
>> << KVM_XICS_PRIORITY_SHIFT;
>> @@ -293,8 +273,9 @@ static int ics_set_kvm_state(ICSState *ics, int
>> version_id)
>> state |= KVM_XICS_QUEUED;
>> }
>>
>> - ret = ioctl(kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
>> - if (ret != 0) {
>> + kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
>> + i + ics->offset, &state, true, &local_err);
>> + if (local_err) {
>> error_report("Unable to restore KVM interrupt controller state"
>> " for IRQs %d: %s", i + ics->offset, strerror(errno));
>> return ret;
>
> This breaks build on CentOS 7.5 with gcc-4.8.5-28.el7_5.1.ppc64le:
>
> hw/intc/xics_kvm.c: In function ‘ics_set_kvm_state’:
> hw/intc/xics_kvm.c:281:13: error: ‘ret’ may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
> return ret;
curiously, a newer compiler gcc7.3.1 didn't see anything.
> Also, if kvm_device_access() has set local_err, then it should be used or
> freed. I think that what you need to do here is something like:
>
>
> ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> i + ics->offset, &state, true, &local_err);
> if (local_err) {
> error_reportf_err(local_err, "Unable to restore KVM interrupt"
> " controller state for IRQs %d: ",
> i + ics->offset);
> return ret;
> }
>
> This also happens to fix the return value of ics_set_kvm_state()
> which is propagated as is to VMState, and thus should be a negative
> errno.
I will see what I can do for that. Cooking a fix.
Thanks,
C.
>> @@ -391,10 +372,6 @@ static void rtas_dummy(PowerPCCPU *cpu,
>> sPAPRMachineState *spapr,
>> int xics_kvm_init(sPAPRMachineState *spapr, Error **errp)
>> {
>> int rc;
>> - struct kvm_create_device xics_create_device = {
>> - .type = KVM_DEV_TYPE_XICS,
>> - .flags = 0,
>> - };
>>
>> if (!kvm_enabled() || !kvm_check_extension(kvm_state,
>> KVM_CAP_IRQ_XICS)) {
>> error_setg(errp,
>> @@ -431,20 +408,19 @@ int xics_kvm_init(sPAPRMachineState *spapr, Error
>> **errp)
>> goto fail;
>> }
>>
>> - /* Create the kernel ICP */
>> - rc = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &xics_create_device);
>> + /* Create the KVM XICS device */
>> + rc = kvm_create_device(kvm_state, KVM_DEV_TYPE_XICS, false);
>> if (rc < 0) {
>> error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
>> goto fail;
>> }
>>
>> - kernel_xics_fd = xics_create_device.fd;
>> -
>> + kernel_xics_fd = rc;
>> kvm_kernel_irqchip = true;
>> kvm_msi_via_irqfd_allowed = true;
>> kvm_gsi_direct_mapping = true;
>>
>> - return rc;
>> + return 0;
>>
>> fail:
>> kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
>
