On 21.03.2024 10:57, Roger Pau Monné wrote: > On Thu, Mar 21, 2024 at 10:17:25AM +0100, Jan Beulich wrote: >> On 21.03.2024 10:10, Roger Pau Monné wrote: >>> On Thu, Mar 21, 2024 at 09:07:10AM +0100, Jan Beulich wrote: >>>> On 20.03.2024 17:29, Roger Pau Monné wrote: >>>>> On Wed, Mar 20, 2024 at 04:09:33PM +0100, Jan Beulich wrote: >>>>>> On 20.03.2024 14:57, Roger Pau Monne wrote: >>>>>>> There's no reason to force HVM guests to have a valid vcpu_info area >>>>>>> when >>>>>>> initializing a vCPU, as the vCPU can also be brought online using the >>>>>>> local >>>>>>> APIC, and on that path there's no requirement for vcpu_info to be setup >>>>>>> ahead >>>>>>> of the bring up. Note an HVM vCPU can operate normally without making >>>>>>> use of >>>>>>> vcpu_info. >>>>>> >>>>>> While I'd agree if you started with "There's no real need to force ...", >>>>>> I >>>>>> still think there is a reason: If one wants to use paravirt interfaces >>>>>> (i.e. >>>>>> hypercalls), they would better do so consistently. After all there's also >>>>>> no need to use VCPUOP_initialise, yet you're not disabling its use. >>>>>> >>>>>> As said in reply to Andrew's reply, besides acting as a sentinel that >>>>>> structure instance also acts as a sink for Xen accesses to a vCPU's >>>>>> vcpu_info. By removing the check, you switch that from being a safeguard >>>>>> to >>>>>> being something that actually has to be expected to be accessed. Unless >>>>>> you've audited all uses to prove that no such access exists. >>>>> >>>>> I'm kind of lost in this last paragraph, how is that different than >>>>> what currently happens when an HVM vCPU >= 32 is brought up using the >>>>> lapic and has no vpcu_info mapped? >>>> >>>> I think this aspect was simply missed back at the time. And I think it >>>> wants mentioning explicitly to justify the change. >>> >>> OK, I can add to the commit message: >>> >>> "Note an HVM vCPU can operate normally without making use of >>> vcpu_info, and in fact does so when brought up from the local APIC." >> >> I'd be fine adding this (or having this added) while committing. >> >>>> As said in reply to Andrew, I don't think the dummy structure can be got >>>> rid of. Nor can the checks here be (easily) removed altogether, i.e. your >>>> change cannot (easily) be extended to PV as well. Even conditional removal >>>> of the structure in !PV builds would first require all vcpu_info accesses >>>> to gain a suitable conditional. Which may be undesirable, as some of these >>>> may be deemed fast paths. >>> >>> I didn't intended to do this here, as replied to Andrew. If we want >>> to get rid of the check for PV also it needs to be done in a different >>> patch, and with a different justification and analysis. >>> >>>>> Also, from a quick look it seems like sites do check whether vcpu_info >>>>> == dummy_vcpu_info, otherwise we would already be in trouble. >>>> >>>> Such checks exist in code managing vcpu_info, but not - afaics - in places >>>> actually accessing it. > > Is there anything else that needs adjusting then, or are you happy to > pick this up and adjust the commit message?
I'll pick this up and adjust (unless Andrew beats me). Jan
