From: Michael Kelley <[email protected]> Sent: Monday, August 25, 2025 2:01 PM > > From: Mukesh R <[email protected]> Sent: Friday, August 22, 2025 > 7:25 PM > > > > On 8/21/25 19:10, Michael Kelley wrote: > > > From: Mukesh R <[email protected]> Sent: Thursday, August 21, > > > 2025 1:50 PM > > >> > > >> On 8/21/25 12:24, Michael Kelley wrote: > > >>> From: Mukesh R <[email protected]> Sent: Wednesday, August > > >>> 20, 2025 7:58 PM > > >>>> > > >>>> On 8/20/25 17:31, Mukesh R wrote: > > >>>>> With time these functions only get more complicated and error prone. > > >>>>> The > > >>>>> saving of ram is very minimal, this makes analyzing crash dumps > > >>>>> harder, > > >>>>> and in some cases like in your patch 3/7 disables unnecessarily in > > >>>>> error case: > > >>>>> > > >>>>> - if (count > HV_MAX_MODIFY_GPA_REP_COUNT) { > > >>>>> - pr_err("Hyper-V: GPA count:%d exceeds supported:%lu\n", count, > > >>>>> - HV_MAX_MODIFY_GPA_REP_COUNT); > > >>>>> + local_irq_save(flags); <<<<<<< > > >>>>> ... > > >>> > > >>> FWIW, this error case is not disabled. It is checked a few lines > > >>> further down as: > > >> > > >> I meant disabled interrupts. The check moves after disabling interrupts, > > >> so > > >> it runs "disabled" in traditional OS terminology :). > > > > > > Got it. But why is it problem to make this check with interrupts disabled? > > > > You are creating disabling overhead where that overhead previously > > did not exist. > > I'm not clear on what you mean by "disabling overhead". The existing code > does the following: > > 1) Validate that "count" is not too big, and return an error if it is. > 2) Disable interrupts > 3) Populate the per-cpu hypercall input arg > 4) Make the hypercall > 5) Re-enable interrupts > > With the patch, steps 1 and 2 are done in a different order: > > 2) Disable interrupts > 1) Validate that "count" is not too big. Re-enable interrupts and return an > error if it is. > 3) Populate the per-cpu hypercall input arg > 4) Make the hypercall > 5) Re-enable interrupts > > Validating "count" with interrupts disabled is probably an additional > 2 or 3 instructions executed with interrupts disabled, which is negligible > compared to the thousands (or more) of instructions the hypercall will > execute with interrupts disabled. > > Or are you referring to something else as "disabling overhead"?
Mukesh -- anything further on what you see as the problem here? I'm just not getting what your concern is. [snip] > > >>>> Furthermore, this makes us lose the ability to permanently map > > >>>> input/output pages in the hypervisor. So, Wei kindly undo. > > >>>> > > >>> > > >>> Could you elaborate on "lose the ability to permanently map > > >>> input/output pages in the hypervisor"? What specifically can't be > > >>> done and why? > > >> > > >> Input and output are mapped at fixed GPA/SPA always to avoid hyp > > >> having to map/unmap every time. > > > > > > OK. But how does this patch set impede doing a fixed mapping? > > > > The output address can be varied depending on the hypercall, instead > > of it being fixed always at fixed address: > > > > *(void **)output = space + offset; <<<<<< > > Agreed. But since mappings from GPA to SPA are page granular, having > such a fixed mapping means that there's a mapping for every byte in > the page containing the GPA to the corresponding byte in the SPA, > right? So even though the offset above may vary across hypercalls, > the output GPA still refers to the same page (since the offset is always > less than 4096), and that page has a fixed mapping. I would expect the > hypercall code in the hypervisor to look for an existing mapping based > on the output page, not the output address that includes the offset. > But I'm haven't looked at the hypervisor code. If the Hyper-V folks say > that a non-zero offset thwarts finding the existing mapping, what does > the hypervisor end up doing? Creating a 2nd mapping wouldn't seem > to make sense. So I'm really curious about what's going on .... > Again, any further information about why we "lose the ability to permanently map input/output pages"? It seems doubtful to me that an offset within the same page would make any difference, but maybe Hyper-V is doing something unexpected. If so, I'd like to know more about what that is. Michael
