Hello! > I'd like to distinguish between the 'missing vgic' and 'something bad > happened when trying to initialize the vgic' cases, which I don't think > we do currently, because the ENXIO code is used in various situations. It is done. Check, for example, vgic_v2_probe(). -ENXIO is returned when some of resources are either missing from DT specification or wrongly given (not page-aligned). In the rest of cases error code is taken from underlying functions, which are more supposed to return things like -EINVAL or -ENOMEM. If you are doubtful, i could suggest to replace -EINVAL with -ENODEV in cases where resources are not present. This would give more clear indication of "we don't have vGIC" condition. > I feel the init flow is relatively difficult to follow and adding a > bunch of flags here and there doesn't seem to help. By adding a > function with a proper comment, it should be more clear, and I don't > like the switch statement on the error return values. Well, the alternative is: 1. If GIC node or vGIC resources are not present, return 0 instead of error code. 2. Implement a function which you suggest. Then: 1. Situation with missing vGIC is just considered to be normal; it's not a to-be-ignored error any more. 2. "vGIC present" situation is automatically determined by vgic_ops != NULL; this means that probe function completely worked and vGIC implementation has been chosen. Agree? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia > -----Original Message----- > From: Christoffer Dall [mailto:christoffer.dall@xxxxxxxxxx] > Sent: Thursday, July 09, 2015 4:25 PM > To: Pavel Fedin > Cc: kvmarm@xxxxxxxxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; 'Marc Zyngier' > Subject: Re: [PATCH 2/2] Detect vGIC presence at runtime > > On Thu, Jul 09, 2015 at 03:50:49PM +0300, Pavel Fedin wrote: > > Hello! > > > > > why not report ENXIO as an error? If probing the vgic fails due to > > > being unable to request the irq or something similar, then surely your > > > system has and error and this should be reported. > > > > It is reported by probe function itself. > > -ENODEV here means there's no GIC at all. -ENXIO happens when, for example, there is GIC node in > > the device tree, but it does not specify vGIC resources. Normally this means that vGIC is defunct on > > the machine. > > > > > > This may be more nicely implemented by letting the vgic init/probe > > > functions set the vgic_present, or maybe better yet, just export a > > > function from vgic.c: > > > > > > bool kvm_vgic_present(void) > > > { > > > return vgic_ops != NULL; > > > } > > > > Is it necessary? Actually this flag is not needed anywhere else except arch/arm/kvm/arm.c, only at > > init time. Runtime should, i believe, use irqchip_in_kernel(), because userland can choose just not > > to use vGIC for some reason (testing for example). > > > I feel the init flow is relatively difficult to follow and adding a > bunch of flags here and there doesn't seem to help. By adding a > function with a proper comment, it should be more clear, and I don't > like the switch statement on the error return values. > > -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
